Re: ALTER TABLE results in "ERROR: could not open relation with OID 43707388"

Manuel Rigger <rigger(dot)manuel(at)gmail(dot)com> writes:
> CREATE TABLE t0(c0 INT);
> CREATE UNIQUE INDEX i0 ON t0(c0);
> ALTER TABLE t0 ADD PRIMARY KEY USING INDEX i0, ALTER c0 TYPE BIGINT;
> -- unexpected: ERROR: could not open relation with OID 43707388

The sequence of events here is that:

1. transformIndexConstraint looks up the index "i0" and saves its OID
in the IndexStmt's indexOid field.

2. ALTER c0 TYPE BIGINT executes first, because of ALTER TABLE's
pass design. It rebuilds the i0 index --- with a new OID.

3. ATExecAddIndexConstraint tries to look up i0 using the old OID.
Kaboom.

Really, it's a horrible idea that parse analysis of the ALTER
commmand is looking up the index at all; that should be postponed
until execution. I tried to refactor things so that we did it
that way, but it turns out that there's an additional side-effect
that happens at parse analysis: if the constraint is PRIMARY KEY
not just UNIQUE, we add implicit SET NOT NULL subcommands to make
sure all the columns are NOT NULL. That's what allows this
example to work:

regression=# create table foo (f1 int);
CREATE TABLE
regression=# create unique index fooi on foo (f1);
CREATE INDEX
regression=# alter table foo add primary key using index fooi;
ALTER TABLE
regression=# \d foo
Table "public.foo"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
f1 | integer | | not null |
Indexes:
"fooi" PRIMARY KEY, btree (f1)

Perhaps we could drop that behavior and insist that you must have
already set up not-nullness of the pkey columns in order to use
ADD PRIMARY KEY USING INDEX. But I bet somebody would complain.

So, in order to fix this properly, we have to postpone the index
lookup into ALTER TABLE execution *and* be willing to generate
SET NOT NULL subcommands on-the-fly during execution.

This seems probably do-able, but it's mighty closely related to
the problems discussed in

https://www.postgresql.org/message-id/flat/10365(dot)1558909428(at)sss(dot)pgh(dot)pa(dot)us

wherein I said

>> 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. ...

So I'm inclined to put this on the back burner until we have some
consensus how to proceed on that.

It seems likely to me that the cleanest fix, for both this issue
and the ADD/DROP GENERATED ones, is to add a new ALTER TABLE pass
that runs after AT_PASS_ALTER_TYPE and AT_PASS_ADD_COL and does
parse analysis activities for subcommands that could depend on the
results of those steps. The parse analysis would result in adding
new subcommands into the queues for AT_PASS_COL_ATTRS,
AT_PASS_ADD_CONSTR, and maybe other late-stage passes. Needing
to run parse analysis activities at this phase is another reason
for extending AlterTable's API as I proposed in that thread ---
in particular, we really want access to the queryString so we
can pass it down to parse analysis for possible use in error messages.

(More generally, I wonder whether we really want initial parse
analysis doing *anything* for ALTER TABLE. Perhaps we ought to
refactor so that we always do that work on-the-fly, one subcommand
at a time.)

regards, tom lane