Oracle branch: review notes
Malcolm Tredinnick
branch merge. So, I sat down tonight with my notes, read through an
updated diff and removed anything that might be evidence I was on drugs
at the time. What follows is what remains. Nothing that is a showstopper
for merging in here.
(1) In core/management.py, there are a lot of lines that are
doing the equivalent of;
if opts.db_tablespace and backend.supports_tablespaces:
# do something....
It would be neater if once, right up front, it was checked if
the backend support tablespaces and, if not, a configuration
error was raised if they have specified a tablespace. It would
make the code a bit neater.
The counter-argument is that it goes against the grain for a
couple of other optional options that don't raise options (e.g.
database host with SQLite), so I'm really only +0 on this and I
suspect I might not get any real support.
(2) The use of django.db.utils.truncate_name() introduces a
small backwards incompatible change for all existing backends.
With sufficiently long table and column names, "manage.py
sqlreset" will not work, because the truncated constraint names
(which are not truncated in trunk or the branch, in fact!) will
be different.
This needs to be notes on the BackwardsIncompatibleChanges page.
(3) In django/dba/models/fields/__init__.py you are still using
the "replace '' with a space" trick once -- around line 162. Is
this valid? I thought we were putting an implicit NULL on blank
fields for Oracle.
(4) In tests/modeltests/tests/datatypes/models.py, remove the
"1." from the heading in that file. That numbering is for the
documentation -- examples of models -- and that test file isn't
a model example (or even an example model), so doesn't need the
numbering. It's borderline whether it's a modeltests or a
regressiontests/ candidates, since it's not really an example of
usage -- which was why we created the regressiontests/
directory; to house such non-examples -- but the purposes for
which the two directories are used has drifted a bit in the last
year, I suspect.
I didn't read through django/db/backends/oracle/ with any real
enthusiasm, because I am by no means an expert (or even a maintainer)
there, so I am not worthy.
I felt a bit queasy reading through all the changes in
django/db/models/query.py, because of all the changes necessary to
handle Oracle's SQL vocabulary; but that's not (entirely) your fault --
we have enough "other database" hacks in there as well. Since I've
started working on the QuerySet refactor again, this reminded me how
much un-fun some parts of that are going to be (and why it would be
better for me if this branch was merged soon).
Anyway, I would encourage Adrian/Jacob to approve merging this. We need
a period of settlement before the next merge (Unicode is ready to merge,
too, but we can't do two too quickly). There are enough changes in
query.py that there might be some subtle bugs (or undocumented
assumptions) creeping in there, so we'll have to keep our eyes open.
Regards,
Malcolm
Jacob Kaplan-Moss
> I suspect my laxness has been holding up the boulder-oracle-sprint
> branch merge.
Yeah, I've also been bad about getting my review notes in. Now that you've
started this rolling, though, I'll just piggy-back off yours. For the most part,
I agree 100% with your assessments, so I'll just add my two bits where needed:
> (2) The use of django.db.utils.truncate_name() introduces a
> small backwards incompatible change for all existing backends.
> With sufficiently long table and column names, "manage.py
> sqlreset" will not work, because the truncated constraint names
> (which are not truncated in trunk or the branch, in fact!) will
> be different.
>
> This needs to be notes on the BackwardsIncompatibleChanges page.
Might it be better to put ``truncate_name()`` into the individual backends? That
would let backends that have no such limits just define it as a no-op.
> I felt a bit queasy reading through all the changes in
> django/db/models/query.py, because of all the changes necessary to
> handle Oracle's SQL vocabulary; but that's not (entirely) your fault --
> we have enough "other database" hacks in there as well. Since I've
> started working on the QuerySet refactor again, this reminded me how
> much un-fun some parts of that are going to be (and why it would be
> better for me if this branch was merged soon).
I suspect that the nausea is mostly my fault; I did that nasty
``backend.get_query_set_class()`` trick because I couldn't think of any way of
pushing more SQL generation into the backend. Malcolm, if you're OK with letting
that code be nasty until you get a chance to refactor, I'm take some Dramamine
and deal with it.
> Anyway, I would encourage Adrian/Jacob to approve merging this. We need
> a period of settlement before the next merge (Unicode is ready to merge,
> too, but we can't do two too quickly). There are enough changes in
> query.py that there might be some subtle bugs (or undocumented
> assumptions) creeping in there, so we'll have to keep our eyes open.
I'm +0.95 on merging now(-ish). My 0.05 hesitation is that I don't have a
working Oracle setup that I can test the branch/merge against. I'd like to be
able to run the complete test suite once I merge. Is there perhaps a VMWare
image of a pre-installed Oracle that I can test this stuff against? Last time I
tried to install it myself I think I fucked everything up :)
Or does one of the other core devs have access to an Oracle install? I'm pretty
sure Adrian doesn't, but if Russ or Malcolm does I'm happy to let one of them do
the merge...
I agree, though; let's get this in.
Jacob
Matt Boersma
wrote:
> ... Is there perhaps a VMWare
> image of a pre-installed Oracle that I can test this stuff against?
Oracle had a developer program called "Oracle By Example" (OBE) that
provides a VMware image with RedHat, Oracle 10gR2 RAC, etc. But I
can't seem to find the actual download for it any longer, and there's
no Oracle image at VMware's "Virtual Appliances" page.
I'd encourage you to revisit
http://www.oracle.com/technology/software/products/database/oracle10g/index.html
and download the .deb or .rpm and give it another try. Oracle
installs used to be tricky, but since they packaged XE this way it's
been painless for me, and obviously having a working setup would be
essential for a buildbot environment. If you see specific errors,
mail me at matt (at) sprout (dot) org and I'll help.
Or, if you're using VMware 6.0 on Linux (host), I'll build you a
Ubuntu or RedHat VMware image with Oracle 10g. Let me know.
And thanks very much for the feedback, Malcolm and Jacob. Ian and I
are digesting it now.
Ian Kelly
On 6/1/07, Malcolm Tredinnick <mal...@pointy-stick.com> wrote:
> (1) In core/management.py, there are a lot of lines that are
> doing the equivalent of;
>
> if opts.db_tablespace and backend.supports_tablespaces:
> # do something....
>
> It would be neater if once, right up front, it was checked if
> the backend support tablespaces and, if not, a configuration
> error was raised if they have specified a tablespace. It would
> make the code a bit neater.
>
> The counter-argument is that it goes against the grain for a
> couple of other optional options that don't raise options (e.g.
> database host with SQLite), so I'm really only +0 on this and I
> suspect I might not get any real support.
Another counter-argument: the db_tablespace option is purely
optimization and doesn't introduce any functional changes, so an error
for when it's unsupported would not be of any value to the users.
That said, I don't really have strong feelings about it either way.
> (2) The use of django.db.utils.truncate_name() introduces a
> small backwards incompatible change for all existing backends.
> With sufficiently long table and column names, "manage.py
> sqlreset" will not work, because the truncated constraint names
> (which are not truncated in trunk or the branch, in fact!) will
> be different.
For the backends that return None for get_max_name_length() (i.e., all
but Oracle and MySQL), the truncate_name call is effectively a no-op,
so this is really only an issue for MySQL. If it's preferable, we
could let MySQL return None as well to retain the old behavior.
> This needs to be notes on the BackwardsIncompatibleChanges page.
Will do.
> (3) In django/dba/models/fields/__init__.py you are still using
> the "replace '' with a space" trick once -- around line 162. Is
> this valid? I thought we were putting an implicit NULL on blank
> fields for Oracle.
We are currently storing empty string as NULL, and I thought all the
old space code was gone (there wasn't much of it). The current diff
doesn't have any changes between lines 99 and 882, so I'm not sure
where you're seeing this.
> (4) In tests/modeltests/tests/datatypes/models.py, remove the
> "1." from the heading in that file. That numbering is for the
> documentation -- examples of models -- and that test file isn't
> a model example (or even an example model), so doesn't need the
> numbering. It's borderline whether it's a modeltests or a
> regressiontests/ candidates, since it's not really an example of
> usage -- which was why we created the regressiontests/
> directory; to house such non-examples -- but the purposes for
> which the two directories are used has drifted a bit in the last
> year, I suspect.
Will do. I think I'll go ahead and move it to regressiontests as
well, because you're correct that it's not an example of usage.
Thanks,
Ian Kelly
Ian Kelly
> On 6/1/07, Malcolm Tredinnick <mal...@pointy-stick.com> wrote:
> > (2) The use of django.db.utils.truncate_name() introduces a
> > small backwards incompatible change for all existing backends.
> > With sufficiently long table and column names, "manage.py
> > sqlreset" will not work, because the truncated constraint names
> > (which are not truncated in trunk or the branch, in fact!) will
> > be different.
>
> For the backends that return None for get_max_name_length() (i.e., all
> but Oracle and MySQL), the truncate_name call is effectively a no-op,
> so this is really only an issue for MySQL. If it's preferable, we
> could let MySQL return None as well to retain the old behavior.
Actually, I just realized what you're pointing out here. The
truncate_name may be a no-op, but the actual construction of the
constraint names prior to truncation has changed; it no longer uses a
hash where it did before. For this particular issue, we should just
revert to the old construction, albeit still calling truncate_name.
Thanks,
Ian Kelly
Russell Keith-Magee
>
> Or does one of the other core devs have access to an Oracle install? I'm pretty
> sure Adrian doesn't, but if Russ or Malcolm does I'm happy to let one of them do
> the merge...
I have access to an Oracle install at work. However, the bigger hurdle
is getting time to do a merge. In one week I have to go on site with a
customer for about 3 weeks... and the place I'm going probably won't
even have telephones, let alone internet. Between now and then, I'll
be frantically trying to get everything ready for the site visit.
Russ %-)
Malcolm Tredinnick
> Hi Malcolm, thanks for taking the time to look this over.
>
> On 6/1/07, Malcolm Tredinnick <mal...@pointy-stick.com> wrote:
> > (1) In core/management.py, there are a lot of lines that are
> > doing the equivalent of;
> >
> > if opts.db_tablespace and backend.supports_tablespaces:
> > # do something....
> >
> > It would be neater if once, right up front, it was checked if
> > the backend support tablespaces and, if not, a configuration
> > error was raised if they have specified a tablespace. It would
> > make the code a bit neater.
> >
> > The counter-argument is that it goes against the grain for a
> > couple of other optional options that don't raise options (e.g.
> > database host with SQLite), so I'm really only +0 on this and I
> > suspect I might not get any real support.
>
> Another counter-argument: the db_tablespace option is purely
> optimization and doesn't introduce any functional changes, so an error
> for when it's unsupported would not be of any value to the users.
> That said, I don't really have strong feelings about it either way.
Let's leave it as is.
>
> > (2) The use of django.db.utils.truncate_name() introduces a
> > small backwards incompatible change for all existing backends.
> > With sufficiently long table and column names, "manage.py
> > sqlreset" will not work, because the truncated constraint names
> > (which are not truncated in trunk or the branch, in fact!) will
> > be different.
>
> For the backends that return None for get_max_name_length() (i.e., all
> but Oracle and MySQL), the truncate_name call is effectively a no-op,
> so this is really only an issue for MySQL. If it's preferable, we
> could let MySQL return None as well to retain the old behavior.
I thought this was the case originally (that it was only existing MySQL
that was backwards-incompat), but then I couldn't see why I thought that
when I was tidying up my notes. Thanks for pointing out the obvious. So
it is backwards incompatible for MySQL only.
>
> > This needs to be notes on the BackwardsIncompatibleChanges page.
>
> Will do.
>
> > (3) In django/dba/models/fields/__init__.py you are still using
> > the "replace '' with a space" trick once -- around line 162. Is
> > this valid? I thought we were putting an implicit NULL on blank
> > fields for Oracle.
>
> We are currently storing empty string as NULL, and I thought all the
> old space code was gone (there wasn't much of it). The current diff
> doesn't have any changes between lines 99 and 882, so I'm not sure
> where you're seeing this.
I was on crack. I had two diff files lying around and I opened up the
wrong one at some point, I suspect (don't worry, most of the notes were
against a diff less than 6 weeks old). You're right -- all gone now.
Regards,
Malcolm
Malcolm Tredinnick
> On 6/1/07, Malcolm Tredinnick <mal...@pointy-stick.com> wrote:
> > I suspect my laxness has been holding up the boulder-oracle-sprint
> > branch merge.
>
> Yeah, I've also been bad about getting my review notes in. Now that you've
> started this rolling, though, I'll just piggy-back off yours. For the most part,
> I agree 100% with your assessments, so I'll just add my two bits where needed:
>
> > (2) The use of django.db.utils.truncate_name() introduces a
> > small backwards incompatible change for all existing backends.
> > With sufficiently long table and column names, "manage.py
> > sqlreset" will not work, because the truncated constraint names
> > (which are not truncated in trunk or the branch, in fact!) will
> > be different.
> >
> > This needs to be notes on the BackwardsIncompatibleChanges page.
>
> Might it be better to put ``truncate_name()`` into the individual backends? That
> would let backends that have no such limits just define it as a no-op.
See my response later in the thread to Ian. /me on drugs -- it only
affects MySQL and they seem to have some idea on how to change/fix that.
If we can (addressing the general audience here), I think we should try
to avoid this change for MySQL -- it's not going to be something that
people think of as "backwards incompatible change" when they stumble
across it, for a start, so it will appear more annoying than it is. But
if we can't avoid it, it's not the end of the world.
>
> > I felt a bit queasy reading through all the changes in
> > django/db/models/query.py, because of all the changes necessary to
> > handle Oracle's SQL vocabulary; but that's not (entirely) your fault --
> > we have enough "other database" hacks in there as well. Since I've
> > started working on the QuerySet refactor again, this reminded me how
> > much un-fun some parts of that are going to be (and why it would be
> > better for me if this branch was merged soon).
>
> I suspect that the nausea is mostly my fault; I did that nasty
> ``backend.get_query_set_class()`` trick because I couldn't think of any way of
> pushing more SQL generation into the backend. Malcolm, if you're OK with letting
> that code be nasty until you get a chance to refactor, I'm take some Dramamine
> and deal with it.
Yeah, this was a throwaway comment, more than anything. As I was reading
it again, I realised it complicated a couple of things for the refactor,
but it's better to know that now than later. We'll survive.
> > Anyway, I would encourage Adrian/Jacob to approve merging this. We need
> > a period of settlement before the next merge (Unicode is ready to merge,
> > too, but we can't do two too quickly). There are enough changes in
> > query.py that there might be some subtle bugs (or undocumented
> > assumptions) creeping in there, so we'll have to keep our eyes open.
>
> I'm +0.95 on merging now(-ish). My 0.05 hesitation is that I don't have a
> working Oracle setup that I can test the branch/merge against. I'd like to be
> able to run the complete test suite once I merge. Is there perhaps a VMWare
> image of a pre-installed Oracle that I can test this stuff against? Last time I
> tried to install it myself I think I fucked everything up :)
Bugger .. you don't have one either? I was hoping to avoid this. I had a
setup last October when I thought I was going to be involved in the
sprint, but that hard-drive has since gone to the great repair man in
the sky.
I'll try to set up a VMware image (clearly going to need that for the
QuerySet work anyway) so that you can share in the joy.
Regards,
Malcolm
Ian Kelly
since been resolved, and I believe the branch is ready to be merged:
1) Many backends.supports_tablespaces checks: we decided to leave this alone.
2) Backwards incompatibility with truncate_name use: this has been
fixed by leaving the name generation alone for the existing backends.
4) The test case has been de-numbered and moved to regressiontests.
I've also discovered that coercing null=True has broken the
Field.get_default method by causing it to return None for string-based
fields instead of the empty string. I fixed it by introducing a
backend check, but the additional backend check then broke the
get_default method for NullBooleanFields to return empty string
instead of None, because for some reason BooleanFields and
NullBooleanFields inherit empty_strings_allowed=True (which Oracle
depends on for the null=True coercion).
I saw no harm in setting NullBooleanField.empty_strings_allowed to
False, so I've gone ahead and done that. I think that the same should
be done for BooleanFields, but I didn't make that change, because:
a) it's currently not broken in any way that I can see;
b) BooleanField defaults the blank argument to True, which would be an
apparent inconsistency;
c) changing it would also change the result of get_default() from
empty string to None in this case.
Thanks,
Ian