Re: Rearranging ALTER TABLE to avoid multi-operations bugs

I wrote:
> Anyway, with the benefit of more time to let this thing percolate
> in my hindbrain, I am thinking that the fundamental error we've made
> is to do transformAlterTableStmt in advance of execution *at all*.
> The idea I now have is to scrap that, and instead apply the
> parse_utilcmd.c transformations individually to each AlterTable
> subcommand when it reaches execution in "phase 2" of AlterTable().

Attached is a patch that does things that way.  This appears to fix
all of the previously reported order-of-operations bugs in ALTER
TABLE, although there's still some squirrely-ness around identity
columns.

My original thought of postponing all parse analysis into the
execution phase turned out to be not quite right.  We still want
to analyze ALTER COLUMN TYPE subcommands before we start doing
anything.  The reason why is that any USING expressions in those
subcommands should all be parsed against the table's starting
rowtype, since those expressions will all be evaluated against
that state during a single rewrite pass in phase 3.  Fortunately
(but not coincidentally, I think) the execution-passes design is
"DROP, then ALTER COLUMN TYPE, then everything else", so that this
is okay.

I had to do some other finagling to get it to work, notably breaking
down some of the passes a bit more.  This allows us to have a rule
that any new subcommands deduced during mid-execution parse analysis
steps will be executed in a strictly later pass.  It might've been
possible to allow it to be "same pass", but I thought that would
be putting an undesirable amount of reliance on the semantics of
appending to a list that some other function is busy scanning.

What I did about the API issues we were arguing about before was
just to move the logic ProcessUtilitySlow had for handling
non-AlterTableStmts generated by ALTER TABLE parse analysis into
a new function that tablecmds.c calls.  This doesn't really resolve
any of the questions I had about event trigger processing, but
I think it at least doesn't make anything worse.  (The event
trigger, logical decoding, and sepgsql tests all pass without
any changes.)  It's tempting to consider providing a similar
API for CREATE SCHEMA to use, but I didn't do so here.

The squirrely-ness around identity is that while this now works:

regression=# CREATE TABLE itest8 (f1 int);
CREATE TABLE
regression=# ALTER TABLE itest8
regression-#   ADD COLUMN f2 int NOT NULL,
regression-#   ALTER COLUMN f2 ADD GENERATED ALWAYS AS IDENTITY;
ALTER TABLE

it doesn't work if there's rows in the table:

regression=# CREATE TABLE itest8 (f1 int);
CREATE TABLE
regression=# insert into itest8 default values;
INSERT 0 1
regression=# ALTER TABLE itest8
  ADD COLUMN f2 int NOT NULL,
  ALTER COLUMN f2 ADD GENERATED ALWAYS AS IDENTITY;
ERROR:  column "f2" contains null values

The same would be true if you tried to do the ALTER as two separate
operations (because the ADD ... NOT NULL, without a default, will
naturally fail on a nonempty table).  So I don't feel *too* awful
about that.  But it'd be better if this worked.  It'll require
some refactoring of where the dependency link from an identity
column to its sequence gets set up.  This patch seems large enough
as-is, and it covers all the cases we've gotten field complaints
about, so I'm content to leave the residual identity issues for later.

			regards, tom lane