Horton

Thread

Commits

  1. Clarify behavior of adding and altering a column in same ALTER command.

  2. Restructure ALTER TABLE execution to fix assorted bugs.

  3. doc: Add best practises section to partitioning docs

  4. Allow on-the-fly capture of DDL event details

  1. Rearranging ALTER TABLE to avoid multi-operations bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-26T22:23:48Z

    We've had numerous bug reports complaining about the fact that ALTER TABLE
generates subsidiary commands that get executed unconditionally, even
if they should be discarded due to an IF NOT EXISTS or other condition;
see e.g. #14827, #15180, #15670, #15710.  In [1] I speculated about
fixing this by having ALTER TABLE maintain an array of flags that record
the results of initial tests for column existence, and then letting it
conditionalize execution of subcommands on those flags.  I started to
fool around with that concept today, and quickly realized that my
original thought of just adding execute-if-this-flag-is-true markers to
AlterTableCmd subcommands was insufficient.  Most of the problems are with
independent commands that execute before or after the main AlterTable,
and would not have any easy connection to state maintained by AlterTable.

The way to fix this, I think, is to provide an AlterTableCmd subcommand
type that just wraps an arbitrary utility statement, and then we can
conditionalize execution of such subcommands using the flag mechanism.
So instead of generating independent "before" and "after" statements,
transformAlterTableStmt would just produce a single AlterTable with
everything in its list of subcommands --- but we'd still use the generic
ProcessUtility infrastructure to execute subcommands that correspond
to existing standalone statements.

Looking into parse_utilcmd.c with an eye to making it do that, I almost
immediately ran across bugs we hadn't even known were there in ALTER TABLE
ADD/DROP GENERATED.  These have got a different but arguably-related
flavor of bug: they are making decisions inside transformAlterTableStmt
that might be wrong by the time we get to execution.  Thus for example

regression=# create table t1 (f1 int);
CREATE TABLE
regression=# alter table t1 add column f2 int not null,
alter column f2 add generated always as identity;
ALTER TABLE
regression=# insert into t1 values(0);
ERROR:  no owned sequence found

This happens because transformAlterTableStmt thinks it can generate
the sequence creation commands for the AT_AddIdentity subcommand,
and also figures it's okay to just ignore the case where the column
doesn't exist.  So we create the column but then we don't make the
sequence.  There are similar bugs in AT_SetIdentity processing, and
I rather suspect that it's also unsafe for AT_AlterColumnType to be
looking at the column's attidentity state --- though I couldn't
demonstrate a bug in that path, because of the fact that 
AT_AlterColumnType executes in a pass earlier than anything that
could change attidentity.

This can't be fixed just by conditionalizing execution of subcommands,
because we need to know the target column's type in order to set up the
sequence correctly.  So what has to happen to fix these things is to
move the decisions, and the creation of the subcommand parsetrees,
into ALTER TABLE execution.

That requires pretty much the same support for recursively calling
ProcessUtility() from AlterTable() that we'd need for the subcommand
wrapper idea.  So I went ahead and tackled it as a separate project,
and attached is the result.

I'm not quite sure if I'm satisfied with the approach shown here.
I made a struct containing the ProcessUtility parameters that need
to be passed down through the recursion, originally with the idea
that this struct might be completely opaque outside utility.c.
However, there's a good deal of redundancy in that approach ---
the relid and stmt parameters of AlterTable() are really redundant
with stuff in the struct.  So now I'm wondering if it would be better
to merge all that stuff and just have the struct as AlterTable's sole
argument.  I'm also not very sure whether AlterTableInternal() ought
to be modified so that it uses or at least creates a valid struct;
it doesn't *need* to do so today, but maybe someday it will.

And the whole thing has a faint air of grottiness about it too.
This makes the minimum changes to what we've got now, but I can't
help thinking it'd look different if we'd designed from scratch.
The interactions with event triggers seem particularly ad-hoc.
It's also ugly that CreateTable's recursion is handled differently
from AlterTable's.

Anybody have thoughts about a different way to approach it?

			regards, tom lane

[1] https://postgr.es/m/7824.1525200461@sss.pgh.pa.us

  2. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Robert Haas <robertmhaas@gmail.com> — 2019-05-29T20:50:49Z

    On Sun, May 26, 2019 at 6:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Anybody have thoughts about a different way to approach it?

I mean, in an ideal world, I think we'd never call back out to
ProcessUtility() from within AlterTable().  That seems like a pretty
clear layering violation.  I assume the reason we've never tried to do
better is a lack of round tuits and/or sufficient motivation.

In terms of what we'd do instead, I suppose we'd try to move as much
as possible inside the ALTER TABLE framework proper and have
everything call into that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  3. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-29T21:52:02Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, May 26, 2019 at 6:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Anybody have thoughts about a different way to approach it?

> I mean, in an ideal world, I think we'd never call back out to
> ProcessUtility() from within AlterTable().  That seems like a pretty
> clear layering violation.  I assume the reason we've never tried to do
> better is a lack of round tuits and/or sufficient motivation.

> In terms of what we'd do instead, I suppose we'd try to move as much
> as possible inside the ALTER TABLE framework proper and have
> everything call into that.

Hm ... I'm not exactly clear on why that would be a superior solution.
It would imply that standalone CREATE INDEX etc would call into the
ALTER TABLE framework --- how is that not equally a layering violation?

Also, recursive ProcessUtility cases exist independently of this issue,
in particular in CreateSchemaCommand.  My worry about my patch upthread
is not really that it introduces another one, but that it doesn't do
anything towards providing a uniform framework/notation for all these
cases.

			regards, tom lane

  4. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Robert Haas <robertmhaas@gmail.com> — 2019-05-29T22:02:35Z

    On Wed, May 29, 2019 at 5:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Hm ... I'm not exactly clear on why that would be a superior solution.
> It would imply that standalone CREATE INDEX etc would call into the
> ALTER TABLE framework --- how is that not equally a layering violation?

Well, the framework could be renamed to something more general, I
suppose, but I don't see a *layering* concern.

From my point of view, the DDL code doesn't do a great job separating
parsing/parse analysis from optimization/execution.  The ALTER TABLE
stuff is actually pretty good in this regard.  But when you build
something that is basically a parse tree and pass it to some other
function that thinks that parse tree may well be coming straight from
the user, you are not doing a good job distinguishing between a
statement and an action which that statement may caused to be
performed.

> Also, recursive ProcessUtility cases exist independently of this issue,
> in particular in CreateSchemaCommand.  My worry about my patch upthread
> is not really that it introduces another one, but that it doesn't do
> anything towards providing a uniform framework/notation for all these
> cases.

I'm not really sure I understand this.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  5. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-29T22:17:46Z

    Robert Haas <robertmhaas@gmail.com> writes:
> From my point of view, the DDL code doesn't do a great job separating
> parsing/parse analysis from optimization/execution.  The ALTER TABLE
> stuff is actually pretty good in this regard.

Meh.  I think a pretty fair characterization of the bug(s) I'm trying to
fix is "we separated parse analysis from execution when we should not
have, because it leads to parse analysis being done against the wrong
database state".  So I'm *very* suspicious of any argument that we should
try to separate them more, let alone that doing so will somehow fix this
set of bugs.

>> Also, recursive ProcessUtility cases exist independently of this issue,
>> in particular in CreateSchemaCommand.  My worry about my patch upthread
>> is not really that it introduces another one, but that it doesn't do
>> anything towards providing a uniform framework/notation for all these
>> cases.

> I'm not really sure I understand this.

Well, I tried to wrap what are currently a random set of ProcessUtility
arguments into one struct to reduce the notational burden.  But as things
are set up, that's specific to the ALTER TABLE case.  I'm feeling like it
should not be, but I'm not very sure where to draw the line between
arguments that should be folded into the struct and ones that shouldn't.

Note that I think there are live bugs in here that are directly traceable
to not having tried to fold those arguments before.  Of the four existing
recursive ProcessUtility calls with context = PROCESS_UTILITY_SUBCOMMAND,
two pass down the outer call's "ParamListInfo params", and two don't ---
how is it not a bug that they don't all behave alike?  And none of the
four pass down the outer call's QueryEnvironment, which seems like even
more of a bug.  So it feels like we ought to have a uniform approach
to what gets passed down during recursion, and enforce it by passing
all such values in a struct rather than as independent arguments.

			regards, tom lane

  6. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    movead.li@highgo.ca <movead.li@highgo.ca> — 2019-06-13T08:33:19Z

    I applied the 'alter-table-with-recursive-process-utility-calls-wip.patch'
on the master(e788e849addd56007a0e75f3b5514f294a0f3bca). And 
when I test the cases, I find it works well on 'alter table t1 add column
f2 int not null, alter column f2 add generated always as identity' case, 
but it doesn't work on #14827, #15180, #15670, #15710.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Here is the test result with #14827 failed
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
postgres=# create table t10 (f1 int);
CREATE TABLE
postgres=# alter table t10 add column f2 int not null,
postgres-# alter column f2 add generated always as identity;
ALTER TABLE
postgres=# 
postgres=# insert into t10 values(0);
INSERT 0 1
postgres=# create table test_serial ( teststring varchar(5));
CREATE TABLE
postgres=# alter table test_serial add column if not exists uid BIGSERIAL;
ALTER TABLE
postgres=# alter table test_serial add column if not exists uid BIGSERIAL;
psql: NOTICE:  column "uid" of relation "test_serial" already exists, skipping
ALTER TABLE
postgres=# 
postgres=# \d
                    List of relations
 Schema |         Name         |   Type   |    Owner     
--------+----------------------+----------+--------------
 public | t10                  | table    | lichuancheng
 public | t10_f2_seq           | sequence | lichuancheng
 public | test_serial          | table    | lichuancheng
 public | test_serial_uid_seq  | sequence | lichuancheng
 public | test_serial_uid_seq1 | sequence | lichuancheng
(5 rows)

postgres=#
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
So it's wrong with a 'test_serial_uid_seq1' sequence to appear.

The new status of this patch is: Waiting on Author

  7. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-07-02T02:00:29Z

    movead li <movead.li@highgo.ca> writes:
> I applied the 'alter-table-with-recursive-process-utility-calls-wip.patch'
> on the master(e788e849addd56007a0e75f3b5514f294a0f3bca). And 
> when I test the cases, I find it works well on 'alter table t1 add column
> f2 int not null, alter column f2 add generated always as identity' case, 
> but it doesn't work on #14827, #15180, #15670, #15710.

This review seems not very on-point, because I made no claim to have fixed
any of those bugs.  The issue at the moment is how to structure the code
to allow ALTER TABLE to call other utility statements --- or, if we aren't
going to do that as Robert seems not to want to, what exactly we're going
to do instead.

The patch at hand does fix some ALTER TABLE ... IDENTITY bugs, because
fixing those doesn't require any conditional execution of utility
statements.  But we'll need infrastructure for such conditional execution
to fix the original bugs.  I don't see much point in working on that part
until we have some agreement about how to handle what this patch is
already doing.

			regards, tom lane

  8. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Thomas Munro <thomas.munro@gmail.com> — 2019-08-01T05:44:16Z

    On Tue, Jul 2, 2019 at 2:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> movead li <movead.li@highgo.ca> writes:
> > I applied the 'alter-table-with-recursive-process-utility-calls-wip.patch'
> > on the master(e788e849addd56007a0e75f3b5514f294a0f3bca). And
> > when I test the cases, I find it works well on 'alter table t1 add column
> > f2 int not null, alter column f2 add generated always as identity' case,
> > but it doesn't work on #14827, #15180, #15670, #15710.
>
> This review seems not very on-point, because I made no claim to have fixed
> any of those bugs.  The issue at the moment is how to structure the code
> to allow ALTER TABLE to call other utility statements --- or, if we aren't
> going to do that as Robert seems not to want to, what exactly we're going
> to do instead.
>
> The patch at hand does fix some ALTER TABLE ... IDENTITY bugs, because
> fixing those doesn't require any conditional execution of utility
> statements.  But we'll need infrastructure for such conditional execution
> to fix the original bugs.  I don't see much point in working on that part
> until we have some agreement about how to handle what this patch is
> already doing.

With my CF manager hat:  I've moved this to the next CF so we can
close this one soon, but since it's really a bug report it might be
good to get more eyeballs on the problem sooner than September.

With my hacker hat:  Hmm.  I haven't looked at the patch, but not
passing down the QueryEnvironment when recursing is probably my fault,
and folding all such things into a new mechanism that would avoid such
bugs in the future sounds like a reasonable approach, if potentially
complicated to back-patch.  I'm hoping to come back and look at this
properly in a while.

-- 
Thomas Munro
https://enterprisedb.com

  9. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    movead.li@highgo.ca <movead.li@highgo.ca> — 2019-08-19T10:57:03Z

    > This review seems not very on-point, because I made no claim to have fixed
> any of those bugs.  The issue at the moment is how to structure the code

I am sorry for that and I have another question now. I researched the related 
code and find something as below:
Code:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
case AT_AddIdentity:
{
...
attnum = get_attnum(relid, cmd->name);
/*
 * if attribute not found, something will error about it
 * later
 */
if (attnum != InvalidAttrNumber)
    generateSerialExtraStmts(&cxt, newdef,
     get_atttype(relid, attnum),def->options, true,
     NULL, NULL);
​...
}
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Test case1:
################################################
create table t10 (f1 int);
alter table t10 add column f2 int not null,
alter column f2 add generated always as identity;
################################################
I find that the value of 'attnum' is 0 because now we do not have the 'f2'
column when I run the Test case1, so it can not generate a sequence
(because it can not run the generateSerialExtraStmts function).
You can see the code annotation that 'something will error about it later',
so I thank it may be an error report instead of executing successfully.

Test case2:
################################################
create table t11 (f1 int);
alter table t11 add column f2 int,
alter column f2 type int8;
################################################ 
Code about 'alter column type' have the same code annotation, and
if you run the Test case2, then you can get an error report. I use Test case2
to prove that it may be an error report instead of executing successfully. 

--
Movead.Li

The new status of this patch is: Waiting on Author

  10. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-09-03T16:04:57Z

    On 2019-Aug-01, Thomas Munro wrote:

> With my hacker hat:  Hmm.  I haven't looked at the patch, but not
> passing down the QueryEnvironment when recursing is probably my fault,
> and folding all such things into a new mechanism that would avoid such
> bugs in the future sounds like a reasonable approach, if potentially
> complicated to back-patch.  I'm hoping to come back and look at this
> properly in a while.

Thomas: Any further input on this?  If I understand you correctly,
you're not saying that there's anything wrong with Tom's patch, just
that you would like to do some further hacking afterwards.

Tom: CFbot says this patch doesn't apply anymore.  Could you please
rebase?  Also: There's further input from Movead; his proposed test
cases might be useful to add.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-09-03T16:21:37Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom: CFbot says this patch doesn't apply anymore.  Could you please
> rebase?

Robert doesn't like the whole approach [1], so I'm not seeing much
point in rebasing the current patch.  The idea I'd been thinking
about instead was to invent a new AlterTableType enum value for
each type of utility command that we can currently generate as a
result of parse analysis of ALTER TABLE, then emit those currently
separate commands as AlterTableCmds with "def" pointing to the
relevant utility-command parsetree, and then add code to ALTER
TABLE to call the appropriate execution functions directly rather
than via ProcessUtility.  (This will add significantly more code
than what I had, and I'm not convinced it's better, just different.)

I haven't gotten to that yet, and now that the CF has started I'm
not sure if I'll have time for it this month.  Maybe we should just
mark the CF entry as RWF for now, or push it out to the next fest.

			regards, tom lane

[1] https://www.postgresql.org/message-id/CA%2BTgmoa3FzZvWriJmqquvAbf8GxrC9YM9umBb18j5M69iuq9bg%40mail.gmail.com

  12. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-29T18:18:05Z

    [ starting to think about this issue again ]

I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I mean, in an ideal world, I think we'd never call back out to
>> ProcessUtility() from within AlterTable().  That seems like a pretty
>> clear layering violation.  I assume the reason we've never tried to do
>> better is a lack of round tuits and/or sufficient motivation.

> ...
> Also, recursive ProcessUtility cases exist independently of this issue,
> in particular in CreateSchemaCommand.  My worry about my patch upthread
> is not really that it introduces another one, but that it doesn't do
> anything towards providing a uniform framework/notation for all these
> cases.

Actually ... looking closer at this, the cases I'm concerned about
*already* do recursive ProcessUtility calls.  Look at utility.c around
line 1137.  The case of interest here is when transformAlterTableStmt
returns any subcommands that are not AlterTableStmts.  As the code
stands, ProcessUtility merrily recurses to itself to handle them.
What I was proposing to do was have the recursion happen from inside
AlterTable(); maybe that's less clean, but surely not by much.

The thing I think you are actually worried about is the interaction
with event triggers, which is already a pretty horrid mess in this
code today.  I don't really follow the comment here about
"ordering of queued commands".  It looks like that comment dates to
Alvaro's commit b488c580a ... can either of you elucidate that?

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().
In that way, the bugs associated with interference between different
AlterTable subcommands touching the same column are removed because
the column's catalog state is up-to-date when we do the parse
transformations.  We can probably also get rid of the problems with
IF NOT EXISTS, because that check would be made in advance of applying
parse transformations for a particular subcommand, and thus its
side-effects would not happen when IF NOT EXISTS fires.  I've not
worked this out in any detail, and there might still be a few ALTER
bugs this framework doesn't fix --- but I think my original idea
of "flags" controlling AlterTable execution probably isn't needed
if we go this way.

Now, if we move things around like that, it will have some effects
on what event triggers see --- certainly the order of operations
at least.  But do we feel a need to retain the same sort of
"encapsulation" that is currently happening due to the aforesaid
logic in utility.c?  I don't fully understand what that's for.

			regards, tom lane

  13. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-10-30T01:10:39Z

    On 2019-Oct-29, Tom Lane wrote:

> The thing I think you are actually worried about is the interaction
> with event triggers, which is already a pretty horrid mess in this
> code today.  I don't really follow the comment here about
> "ordering of queued commands".  It looks like that comment dates to
> Alvaro's commit b488c580a ... can either of you elucidate that?

The point of that comment is that if you enqueue the commands as they
are returned by pg_event_trigger_ddl_commands() (say by writing them to
a table) they must be emitted in an order that allows them to be
re-executed in a remote server that duplicates this one, and the final
state should be "the same".

> Now, if we move things around like that, it will have some effects
> on what event triggers see --- certainly the order of operations
> at least.  But do we feel a need to retain the same sort of
> "encapsulation" that is currently happening due to the aforesaid
> logic in utility.c?  I don't fully understand what that's for.

Sadly, the DDL replay logic is not being used for anything at present,
so I don't have a good test case to ensure that a proposed change is
good in this regard.  I've been approached by a couple people interested
in finishing the DDL conversion thing, but no takers so far.  I know
there's people using code based on the src/test/modules/test_ddl_deparse
module, but not for replicating a server's state to a different server, as
far as I know.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  14. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-30T05:44:05Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Oct-29, Tom Lane wrote:
>> The thing I think you are actually worried about is the interaction
>> with event triggers, which is already a pretty horrid mess in this
>> code today.  I don't really follow the comment here about
>> "ordering of queued commands".  It looks like that comment dates to
>> Alvaro's commit b488c580a ... can either of you elucidate that?

> The point of that comment is that if you enqueue the commands as they
> are returned by pg_event_trigger_ddl_commands() (say by writing them to
> a table) they must be emitted in an order that allows them to be
> re-executed in a remote server that duplicates this one, and the final
> state should be "the same".

Hm.  I don't think I understand what is the use-case behind all this.
If "ALTER TABLE tab DO SOMETHING" generates some subcommands to do what
it's supposed to do, and then an event trigger is interested in replaying
that ALTER, how is it supposed to avoid having the subcommands happen
twice?  That is, it seems like we'd be better off to suppress the
generated subcommands from the event stream, because they'd just get
generated again anyway from execution of the primary command.  Or, if
there's something that is interested in knowing that those subcommands
happened, that's fine, but they'd better be marked somehow as informative
rather than something you want to explicitly replay.  (And if they are
just informative, why is the ordering so critical?)

			regards, tom lane

  15. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-01T22:26:15Z

    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

  16. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-13T20:02:26Z

    I wrote:
> [ fix-alter-table-order-of-operations-1.patch ]

The cfbot noticed that this failed to apply over a recent commit,
so here's v2.  No substantive changes.

			regards, tom lane

  17. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-26T17:47:31Z

    I wrote:
>> [ fix-alter-table-order-of-operations-1.patch ]
> The cfbot noticed that this failed to apply over a recent commit,
> so here's v2.  No substantive changes.

Another rebase required :-(.  Still no code changes from v1, but this
time I remembered to add a couple more test cases that I'd been
meaning to put in, mostly based on bug reports from Manuel Rigger.

I'd kind of like to get this cleared out of my queue soon.
Does anyone intend to review it further?

			regards, tom lane

  18. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-01-14T22:27:01Z

    I wrote:
> [ fix-alter-table-order-of-operations-3.patch ]

Rebased again, fixing a minor conflict with f595117e2.

> I'd kind of like to get this cleared out of my queue soon.
> Does anyone intend to review it further?

If I don't hear objections pretty darn quick, I'm going to
go ahead and push this.

			regards, tom lane

  19. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Sergei Kornilov <sk@zsrv.org> — 2020-01-15T16:15:44Z

    Hello

Thank you!

I am clearly not a good reviewer for such changes... But for a note: I read the v4 patch and have no useful comments. Good new tests, reasonable code changes to fix multiple bug reports.

The patch is proposed only for the master branch, right?

regards, Sergei

  20. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-01-15T16:32:38Z

    Sergei Kornilov <sk@zsrv.org> writes:
> I am clearly not a good reviewer for such changes... But for a note: I read the v4 patch and have no useful comments. Good new tests, reasonable code changes to fix multiple bug reports.

Thanks for looking!

> The patch is proposed only for the master branch, right?

Yes, it seems far too risky for the back branches.

			regards, tom lane

  21. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-01-15T17:11:57Z

    On 2020-Jan-14, Tom Lane wrote:

> I wrote:
> > [ fix-alter-table-order-of-operations-3.patch ]
> 
> Rebased again, fixing a minor conflict with f595117e2.
> 
> > I'd kind of like to get this cleared out of my queue soon.
> > Does anyone intend to review it further?
> 
> If I don't hear objections pretty darn quick, I'm going to
> go ahead and push this.

I didn't review in detail, but it seems good to me.  I especially liked
getting rid of the ProcessedConstraint code, and the additional test
cases.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  22. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-01-15T18:12:56Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I didn't review in detail, but it seems good to me.  I especially liked
> getting rid of the ProcessedConstraint code, and the additional test
> cases.

Thanks for looking!

Yeah, all those test cases expose situations where we misbehave
today :-(.  I wish this were small enough to be back-patchable,
but it's not feasible.

			regards, tom lane

  23. Re: Rearranging ALTER TABLE to avoid multi-operations bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-01-21T00:46:52Z

    I wrote:
> 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.

After further poking at that, I've concluded that maybe this is not
a bug but operating as designed.  Adding the GENERATED property in a
separate step is arguably equivalent to setting a plain default in
a separate step, and look at how we handle that:

regression=# create table t1(x int);
CREATE TABLE
regression=# insert into t1 values(1);
INSERT 0 1
regression=# alter table t1 add column y int default 11,
  alter column y set default 12;
ALTER TABLE
regression=# table t1;
 x | y  
---+----
 1 | 11
(1 row)

This is documented, rather opaquely perhaps, for the SET DEFAULT
case:

SET/DROP DEFAULT

    These forms set or remove the default value for a column. Default
    values only apply in subsequent INSERT or UPDATE commands; they do not
    cause rows already in the table to change.

So the design principle here seems to be that we fill the column
using whatever is specified *in the ADD COLUMN subcommand*, and
any screwing-about in other subcommands just affects what the
behavior will be in subsequent INSERT commands.  That's a little
weird but it has potential use-cases.  If we attempt to apply the
"new" default immediately then this syntax devolves to having the
same effects as a simple ADD-COLUMN-with-default.  There's not
a lot of reason to write the longer form if that's what you wanted.

So I'm now inclined to think that the code is all right.  We could
improve the documentation, perhaps, with an explicit example.
Also, the man page's entry for SET GENERATED says nothing of this,
but it likely ought to say the same thing as SET DEFAULT.

Also, we don't really have any test cases proving it works that way.
Simple tests, such as the one above, are not too trustworthy because
the attmissingval optimization tends to hide what's really happening.
(I found this out the hard way while messing with a patch to change
the behavior --- which I now think we shouldn't do, anyhow.)

			regards, tom lane