Re: ALTER TABLE with ADD COLUMN and ADD PRIMARY KEY USING INDEX throws spurious "column contains null values"

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Apr 17, 2019 at 11:55 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> * I'm not sure whether we want to try to back-patch this, or how
>> far it should go. The misbehavior has been there a long time
>> (at least back to 8.4, I didn't check further); so the lack of
>> previous reports means people seldom try to do it. That may
>> indicate that it's not worth taking any risks of new bugs to
>> squash this one. (Also, I suspect that it might take a lot of
>> work to port this to before v10, because there are comments
>> suggesting that this code worked a good bit differently before.)
>> I do think we should shove this into v12 though.

> Shoving it into v12 but not back-patching seems like a reasonable
> compromise, although I have not reviewed the patch or tried to figure
> out how risky it is.

Here's a less-WIP patch for that. I fixed up some more stuff:

>> * I initially thought that index_check_primary_key could be simplified
>> to the point where it *only* throws an error for missing NOT NULL.
>> This soon proved to be wrong, because the comments for the function
>> are lies, or at least obsolete: there are multiple scenarios in which
>> a CREATE TABLE with a PRIMARY KEY option does need this function to
>> perform ALTER TABLE SET NOT NULL.

I decided that a cleaner way to handle this was to make the parser
generate required ALTER TABLE SET NOT NULL operations in all cases,
not just the ALTER TABLE case. This gets rid of the former confused
situation wherein transformIndexConstraint forced primary-key columns
NOT NULL in some situations and abdicated responsibility in others.

>> * We need to fix the order of operations in ALTER TABLE phase 2 so that
>> any AT_SetNotNull subcommands happen after the AT_PASS_ADD_COL pass
>> (else the column might not be there yet) and before AT_PASS_ADD_INDEX
>> (else index_check_primary_key will complain). I did this by putting
>> AT_SetNotNull into the AT_PASS_COL_ATTRS pass and moving that to after
>> AT_PASS_ADD_COL; it had been before AT_PASS_ADD_COL, but that seems at
>> best random and at worst broken. (AT_SetIdentity is the only existing
>> subcommand using AT_PASS_COL_ATTRS, and it sure seems like it'd make more
>> sense to run it after AT_PASS_ADD_COL, so that it can work on a column
>> being added in the same ALTER. Am I missing something?)

Sure enough, AT_SetIdentity is broken for the case where the column was
just created in the same ALTER command, as per test case added below.
Admittedly, that's a fairly unlikely thing to do, but it should work;
so the current ordering of these passes is wrong.

BTW, now that we have an AT_PASS_COL_ATTRS pass, it's a bit tempting to
shove other stuff that's in the nature of change-a-column-attribute into
it; there are several AT_ subcommands of that sort that are currently in
AT_PASS_MISC. I didn't do that here though.

Also, this fixes the issue complained of in
https://postgr.es/m/16115.1555874162@sss.pgh.pa.us

Barring objections I'll commit this tomorrow or so.

regards, tom lane