ALTER COLUMN ... WITH DEFAULT NULL does not change the default

• Key: DERBY-4006
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Bryan Pendleton
• Reporter: Knut Anders Hatlen
• Component/s: SQL
• Affects Version/s: 10.2.2.0, 10.3.1.4, 10.3.2.1, 10.3.3.0, 10.4.1.3, 10.4.2.0
• Fix Version/s: 10.4.2.1, 10.5.1.1

Description

Reported on derby-user.
http://mail-archives.apache.org/mod_mbox/db-derby-user/200901.mbox/[e-mail]%3e

ij> create table t (x varchar(5) default 'abc');
0 rows inserted/updated/deleted
ij> alter table t alter column x with default null;
0 rows inserted/updated/deleted
ij> insert into t values default;
1 row inserted/updated/deleted
ij> select * from t;
X
-----
abc

1 row selected

Comments

[Knut Anders Hatlen]

This is a regression introduced between 10.2.1.6 and 10.2.2.0.

[Knut Anders Hatlen]

It looks like the behaviour changed after this commit:

------------------------------------------------------------------------
r474502 | bpendleton | 2006-11-13 21:36:51 +0100 (Mon, 13 Nov 2006) | 13 lines

DERBY-1495: Error modifying an identity column after altering the column
DERBY-1645: ALTER TABLE SET INCREMENT turns off "Generated By Default"

This patch changes ModifyColumnNode.bindAndValidateDefault so that it
detects the case(s) where the user is altering aspects of an identity column,
and ensures that the other aspects of that identity column are preserved and
not lost. The crucial issue is that if the column is Generated By Default,
then the DefaultInfoImpl column in the SYSCOLUMNS table needs to get
retained when the user uses ALTER TABLE to change either the start value
or the increment value; otherwise the behavior effectively switches from
Generated By Default to Generated Always.


------------------------------------------------------------------------

[Bryan Pendleton]

Thanks for tracking this down, Knut. I'll have a look and see if I can figure out what broke.

[Bryan Pendleton]

I think that "default NULL" is a very special case, and ModifyColumnNode is
having trouble distinguishing between the case where the user didn't
specify a default at all, versus the case where they explicitly specified a default
value of NULL.

I think that the easiest thing is probably to enhance ModifyColumnNode to
have a boolean which states whether the user did or did not explicitly
specify a default value, so that we can change line 346 of ModifyColumnNode from

  if (defaultNode == null)

to

  if (! statementExplicitlySpecifiedDefaultForColumn)

[Knut Anders Hatlen]

There is a method in sqlgrammar.jj called defaultNullOnlyClause() which appears to be meant for this case. It's currently not used, though.

[Knut Anders Hatlen]

Shouldn't defaultNode be an object of type UntypedNullConstantNode in this case, and not null? At least, that's how I understand the code.

[Knut Anders Hatlen]

Or actually... It seems like the defaultNode argument (with declared type Object) passed to ColumnDefinitionNode.init() is an UntypedNullConstantNode, but the instance variable defaultNode is declared to be of type DefaultNode, and it cannot hold an UntypedNullConstantNode, so it should be null in this case.

Adding a boolean sounds like a good idea. +1

[Bryan Pendleton]

Attached is 'quickCodePatch.diff', which seems to make the test case pass.

I haven't done any additional testing, and the patch is not for commit, just
to give an idea of a possible approach to a fix.

I'll explore a full patch based on this idea, but would welcome any feedback
in the meantime.

[Knut Anders Hatlen]

The check for (defaultNode == null) in ModifyColumnNode was supposed to mean "no default value was specified". Ignoring for a moment that we don't currently have syntax for it in ALTER TABLE, a default value in a ModifyColumnNode could also be a generation clause, in which case the instance variable defaultNode is also null. So I'm wondering if the new check for (defaultNode == null && !defaultNULLExplicitlySpecified) still doesn't express exactly what we want to check (although I'm convinced that it fixes the bug and does not cause any problems with the currently accepted syntax for ALTER TABLE).

Do you think it would make sense to instead have a boolean called keepCurrentDefault that we initialized in ColumnDefinitionNode.init() like this? (note the difference between the local variable defaultNode being null and the instance variable defaultNode being null)

    keepCurrentDefault = (defaultNode == null);

Then the check in ModifyColumnNode would simply be "if (keepCurrentDefaultValue) ...". This way we check that "no default is associated with this ColumnDefinitionNode" rather than "no DefaultNode or UntypedNullConstantNode is associated with this ModifyColumnNode". Don't know if this makes any sense at all, though, but I thought I'd raise the issue.

[Bryan Pendleton]

Thanks Knut, that seems like a cleaner wording for the variable name.

Meanwhile, my regression test run with the 'quickCodePatch' applied did not
find any additional problems.

I'll re-work the patch slightly, incorporating your ideas, adding some comments
and some tests, and be back later with a reviewable patch.

[Bryan Pendleton]

Updated the patch with some early review comments and added a test.

Please have a look at 'patchWithTest.diff' at your convenience.

[Knut Anders Hatlen]

The patch looks good to me, and thanks for writing such a clear and detailed comment.
+1 to commit if the regression tests pass.

[Knut Anders Hatlen]

I played some more with the patch and didn't find any problems. I don't know if it warrants an extra test case, but I found another manifestation of the bug fixed by the patch. Without the patch, this does not raise an error:

ij> alter table t add column y int generated always as (-1);
0 rows inserted/updated/deleted
ij> alter table t alter column y default null;
0 rows inserted/updated/deleted

Even though it doesn't raise an error, it doesn't actually change the default value. Specifying a non-null default would raise an error even without the patch.

With the patch, it does raise an error:

ij> alter table t add column y int generated always as (-1);
0 rows inserted/updated/deleted
ij> alter table t alter column y default null;
ERROR 42XA7: 'Y' is a generated column. You cannot change its default value.

(Strangely though, if the column is GENERATED ALWAYS AS IDENTITY, you are allowed to change the default value. But that's not related to this issue.)

[Bryan Pendleton]

Those test cases seem quite useful and relevant to me, thanks for sending them along.

I've updated the patch, and verified that the regression tests pass.

If there are no additional comments, I'll submit addGenColTest.diff to the
trunk over the next few days.

[Bryan Pendleton]

Committed addGenColTest.diff to the trunk as revision 733401.

If no new problems show up, I'll investigate merging this change to the 10.4 branch.

[Bryan Pendleton]

In order to successfully merge this patch back to the 10.4 branch,
the tests for the ALTER COLUMN behavior on generated columns
must be removed, as 10.4 does not have generated column support.

[Bryan Pendleton]

Attached 'tenFourPatch.diff' is the patch I propose to apply to the 10.4 branch.

The only difference between this patch and the mainline patch is the
removal of the "generated as (-1)" test case from the autoincrement suite.

If my regression runs are clean, I intend to submit this patch to 10.4 this week.

[Bryan Pendleton]

Submitted the 10.4 change as revision 734585.

Is there a need to merge this change to other branches? I'm satisfied having
committed the fix to the trunk and to the 10.4 branch, so I propose to resolve
this issue at this point. Please let me know if you think more work is necessary
at this time.

[Knut Anders Hatlen]

Thanks for fixing this, Bryan. I've verified that it works on head of trunk and 10.4 now. I'm happy with having the fix on those two branches, but anyone should feel free to merge it to 10.3 and 10.2 as well if they think that's required. It might be worth the extra effort to ensure that we don't have any known regressions on the stable branches.

[Bryan Pendleton]

Thanks Knut for all the help on this. If anyone decides later on that they wish
to have this fix in an earlier branch, I think it should be a straightforward
merge and I'd be glad to help. For the time being, though, I'm marking the
issue as resolved.