Re: Rearranging ALTER TABLE to avoid multi-operations bugs

[ starting to think about this issue again ]

I wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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