Обсуждение: ALTER TABLE results in "ERROR: could not open relation with OID 43707388"

Hi everyone,

The test case below unexpectedly results in "ERROR: could not open
relation with OID 43707388" (or some other number).

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

I would expect that the statements are equivalent to the following,
where the ALTER TABLE actions were split up, and which execute without
errors:

CREATE TABLE t0(c0 INT);
CREATE UNIQUE INDEX i0 ON t0(c0);
ALTER TABLE t0 ADD PRIMARY KEY USING INDEX i0;
ALTER TABLE t0 ALTER c0 TYPE BIGINT; -- no error

I'm using the following Postgres version: psql (11.4 (Ubuntu
11.4-1.pgdg19.04+1)).

Best,
Manuel

Hi David,

Thanks for the quick response! Do you know which commit fixed it?

Best,
Manuel


On Thu, Jul 4, 2019 at 6:24 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> On Thu, Jul 4, 2019 at 7:42 AM Manuel Rigger <rigger.manuel@gmail.com> wrote:
>>
>> Hi everyone,
>>
>> I would expect that the statements are equivalent to the following,
>> where the ALTER TABLE actions were split up, and which execute without
>> errors:
>>
>> I'm using the following Postgres version: psql (11.4 (Ubuntu
>> 11.4-1.pgdg19.04+1)).
>
>
> This has been fixed for version 11.5 which is as yet unreleased.
>
> David J.
>

Thanks again!

Best,
Manuel

On Thu, Jul 4, 2019 at 7:07 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> On Thu, Jul 4, 2019 at 9:41 AM Manuel Rigger <rigger.manuel@gmail.com> wrote:
>>
>> Thanks for the quick response! Do you know which commit fixed it?
>
>
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=da1041fc3a2b65a6a36f1b8b91765a46e54e571e
>
> David J.
>

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Thu, Jul 4, 2019 at 7:42 AM Manuel Rigger <rigger.manuel@gmail.com>
> wrote:
>> I would expect that the statements are equivalent to the following,
>> where the ALTER TABLE actions were split up, and which execute without
>> errors:

> This has been fixed for version 11.5 which is as yet unreleased.

Really?  Cause it still fails in HEAD for me.  It might be related to
that new-in-11.4 bug, or to the old bug we were trying to fix, but
I fear it's a distinct problem.  I've not had time to dig into the code
though.

            regards, tom lane

On Thu, Jul 04, 2019 at 06:50:54PM -0400, Tom Lane wrote:
> Really?  Cause it still fails in HEAD for me.  It might be related to
> that new-in-11.4 bug, or to the old bug we were trying to fix, but
> I fear it's a distinct problem.  I've not had time to dig into the code
> though.

This is failing on HEAD (d5ab9df), 11.3 and 11.4.  So that looks like
a separate, not fixed, issue to me.
--
Michael

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Thu, Jul 4, 2019 at 7:42 AM Manuel Rigger <rigger.manuel@gmail.com>
> wrote:
>> I would expect that the statements are equivalent to the following,
>> where the ALTER TABLE actions were split up, and which execute without
>> errors:

> This has been fixed for version 11.5 which is as yet unreleased.

Really?  Cause it still fails in HEAD for me.  It might be related to
that new-in-11.4 bug, or to the old bug we were trying to fix, but
I fear it's a distinct problem.  I've not had time to dig into the code
though.

Manuel Rigger <rigger.manuel@gmail.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.1558909428@sss.pgh.pa.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

I wrote:
> Manuel Rigger <rigger.manuel@gmail.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.

FYI, the patch I posted at

https://www.postgresql.org/message-id/24573.1572647175%40sss.pgh.pa.us

fixes this (as well as your other similar complaint).  I forgot to
add a regression test matching this case, but will do so later.

            regards, tom lane

Thanks a lot for the patch and for informing me about the fix!

Best,
Manuel

On Sat, Nov 2, 2019 at 5:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > Manuel Rigger <rigger.manuel@gmail.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.
>
> FYI, the patch I posted at
>
> https://www.postgresql.org/message-id/24573.1572647175%40sss.pgh.pa.us
>
> fixes this (as well as your other similar complaint).  I forgot to
> add a regression test matching this case, but will do so later.
>
>                         regards, tom lane