pgsql: Restructure ALTER TABLE execution to fix assorted bugs. - Mailing list pgsql-committers

Restructure ALTER TABLE execution to fix assorted bugs.

We've had numerous bug reports about how (1) IF NOT EXISTS clauses in
ALTER TABLE don't behave as-expected, and (2) combining certain actions
into one ALTER TABLE doesn't work, though executing the same actions as
separate statements does.  This patch cleans up all of the cases so far
reported from the field, though there are still some oddities associated
with identity columns.

The core problem behind all of these bugs is that we do parse analysis
of ALTER TABLE subcommands too soon, before starting execution of the
statement.  The root of the bugs in group (1) is that parse analysis
schedules derived commands (such as a CREATE SEQUENCE for a serial
column) before it's known whether the IF NOT EXISTS clause should cause
a subcommand to be skipped.  The root of the bugs in group (2) is that
earlier subcommands may change the catalog state that later subcommands
need to be parsed against.

Hence, postpone parse analysis of ALTER TABLE's subcommands, and do
that one subcommand at a time, during "phase 2" of ALTER TABLE which
is the phase that does catalog rewrites.  Thus the catalog effects
of earlier subcommands are already visible when we analyze later ones.
(The sole exception is that we do parse analysis for ALTER COLUMN TYPE
subcommands during phase 1, so that their USING expressions can be
parsed against the table's original state, which is what we need.
Arguably, these bugs stem from falsely concluding that because ALTER
COLUMN TYPE must do early parse analysis, every other command subtype
can too.)

This means that ALTER TABLE itself must deal with execution of any
non-ALTER-TABLE derived statements that are generated by parse analysis.
Add a suitable entry point to utility.c to accept those recursive
calls, and create a struct to pass through the information needed by
the recursive call, rather than making the argument lists of
AlterTable() and friends even longer.

Getting this to work correctly required a little bit of fiddling
with the subcommand pass structure, in particular breaking up
AT_PASS_ADD_CONSTR into multiple passes.  But otherwise it's mostly
a pretty straightforward application of the above ideas.

Fixing the residual issues for identity columns requires refactoring of
where the dependency link from an identity column to its sequence gets
set up.  So that seems like suitable material for a separate patch,
especially since this one is pretty big already.

Discussion: https://postgr.es/m/10365.1558909428@sss.pgh.pa.us

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/1281a5c907b41e992a66deb13c3aa61888a62268

Modified Files
--------------
src/backend/commands/tablecmds.c                   | 494 ++++++++++++++++-----
src/backend/commands/view.c                        |   4 +
src/backend/parser/parse_utilcmd.c                 | 142 +++---
src/backend/tcop/utility.c                         | 115 ++---
src/include/commands/tablecmds.h                   |   5 +-
src/include/nodes/parsenodes.h                     |   2 -
src/include/parser/parse_utilcmd.h                 |   6 +-
src/include/tcop/utility.h                         |  13 +
.../modules/test_ddl_deparse/test_ddl_deparse.c    |   3 -
src/test/regress/expected/alter_table.out          | 160 ++++++-
src/test/regress/expected/identity.out             |  62 +++
src/test/regress/sql/alter_table.sql               |  50 ++-
src/test/regress/sql/identity.sql                  |  38 ++
13 files changed, 827 insertions(+), 267 deletions(-)