BUG #15631: Generated as identity field in a temporary table with on commit drop corrupts system catalogs

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

BUG #15631: Generated as identity field in a temporary table with on commit drop corrupts system catalogs

apt.postgresql.org Repository Update
The following bug has been logged on the website:

Bug reference:      15631
Logged by:          Serge Latyntsev
Email address:      [hidden email]
PostgreSQL version: 11.1
Operating system:   Alpine
Description:        

When using `generated by default as identity` in a temporary table with `on
commit drop`, but without starting a transaction, system catalogs get
corrupted and won't let create temporary tables anymore.

Here's how to reproduce (within a docker container):

$ docker exec -it $(docker run -d --rm postgres:11.1-alpine) bash
# psql postgres postgres
# create temporary table foo ( bar int generated by default as identity ) on
commit drop;
CREATE TABLE
# \q
# psql postgres postgres
# create temporary table a (b varchar);
ERROR:  could not open relation with OID 16388
LINE 1: create temporary table a (b varchar);

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15631: Generated as identity field in a temporary table with on commit drop corrupts system catalogs

Tom Lane-2
PG Bug reporting form <[hidden email]> writes:
> When using `generated by default as identity` in a temporary table with `on
> commit drop`, but without starting a transaction, system catalogs get
> corrupted and won't let create temporary tables anymore.

Interesting.  I can reproduce that (don't need the docker bit...).
The temp table goes away, but the sequence is still there, and so
are its pg_depend entries --- including one saying that it depends
on the table.  That bollixes later attempts to clean out the temp
namespace, since deletion tries to recurse to the missing table.

Even more interesting, it works if you wrap the CREATE in begin/commit.

I don't have time to probe further right now, but I believe what is
wrong is that we're missing a CommandCounterIncrement call after the
sequence is created, or at least after its internal dependency on the
table column is created.  When the transaction commits immediately,
dependency.c fails to see the internal dependency entry and so it
doesn't remove the sequence.  Wrapped in a transaction, there's a
CCI somewhere and all is well.

Peter, will you take this?

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15631: Generated as identity field in a temporary table with on commit drop corrupts system catalogs

Peter Eisentraut-6
On 11/02/2019 21:33, Tom Lane wrote:
> I don't have time to probe further right now, but I believe what is
> wrong is that we're missing a CommandCounterIncrement call after the
> sequence is created, or at least after its internal dependency on the
> table column is created.  When the transaction commits immediately,
> dependency.c fails to see the internal dependency entry and so it
> doesn't remove the sequence.  Wrapped in a transaction, there's a
> CCI somewhere and all is well.

Right.  I think it would be good to put a CommandCounterIncrement() at
the top of PreCommit_on_commit_actions().  That ensures that the
dependency code always see the latest state.

> That bollixes later attempts to clean out the temp
> namespace, since deletion tries to recurse to the missing table.

Should there be some warnings when this happens?

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15631: Generated as identity field in a temporary table with on commit drop corrupts system catalogs

Tom Lane-2
Peter Eisentraut <[hidden email]> writes:
> On 11/02/2019 21:33, Tom Lane wrote:
>> I don't have time to probe further right now, but I believe what is
>> wrong is that we're missing a CommandCounterIncrement call after the
>> sequence is created, or at least after its internal dependency on the
>> table column is created.  When the transaction commits immediately,
>> dependency.c fails to see the internal dependency entry and so it
>> doesn't remove the sequence.  Wrapped in a transaction, there's a
>> CCI somewhere and all is well.

> Right.  I think it would be good to put a CommandCounterIncrement() at
> the top of PreCommit_on_commit_actions().  That ensures that the
> dependency code always see the latest state.

Hm, I'd be more inclined to find where the sequence creation is happening
and add a CCI at the end, because that comports better with the general
plan for inserting CCIs.  There may be other issues of this same sort with
doing-X-just-after-identity-sequence-creation if you don't fix it that
way.

>> That bollixes later attempts to clean out the temp
>> namespace, since deletion tries to recurse to the missing table.

> Should there be some warnings when this happens?

Like what?

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15631: Generated as identity field in a temporary table with on commit drop corrupts system catalogs

Michael Paquier-2
On Tue, Feb 12, 2019 at 09:27:50AM -0500, Tom Lane wrote:
> Hm, I'd be more inclined to find where the sequence creation is happening
> and add a CCI at the end, because that comports better with the general
> plan for inserting CCIs.  There may be other issues of this same sort with
> doing-X-just-after-identity-sequence-creation if you don't fix it that
> way.

Agreed.  I don't think that it is the correct logic to put an
after-the-fact CCI just before executing any drop or truncate actions.
It should happen after the creation of the new object so as it becomes
correctly visible within the transaction.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: BUG #15631: Generated as identity field in a temporary table with on commit drop corrupts system catalogs

Michael Paquier-2
On Wed, Feb 13, 2019 at 11:38:17AM +0900, Michael Paquier wrote:
> Agreed.  I don't think that it is the correct logic to put an
> after-the-fact CCI just before executing any drop or truncate actions.
> It should happen after the creation of the new object so as it becomes
> correctly visible within the transaction.

The problem comes from process_owned_by() in sequence.c which has
added in v10 some handling for internal dependencies in the case of an
identity sequence, and the dependency link between the sequence and
its relation is added there.

Another thing I was wondering is if we should add the CCI directly to
recordMultipleDependencies() for internal dependencies, still it seems
to me that it would be an overkill as some dependency registrers may
do the CCI by themselves after adding the pg_depend link and doing
some other operations, so I discarded the idea.

The patch attached solves the problem, for consistency I would suggest
doing the CCI even for auto dependencies.
--
Michael

identity-depend-cci.patch (678 bytes) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: BUG #15631: Generated as identity field in a temporary table with on commit drop corrupts system catalogs

Michael Paquier-2
On Wed, Feb 13, 2019 at 01:41:05PM +0900, Michael Paquier wrote:
> The patch attached solves the problem, for consistency I would suggest
> doing the CCI even for auto dependencies.

I have added this proposal of bug fix to the next CF for now:
https://commitfest.postgresql.org/22/1997/
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: BUG #15631: Generated as identity field in a temporary table with on commit drop corrupts system catalogs

Peter Eisentraut-6
In reply to this post by Michael Paquier-2
On 2019-02-13 05:41, Michael Paquier wrote:

> On Wed, Feb 13, 2019 at 11:38:17AM +0900, Michael Paquier wrote:
>> Agreed.  I don't think that it is the correct logic to put an
>> after-the-fact CCI just before executing any drop or truncate actions.
>> It should happen after the creation of the new object so as it becomes
>> correctly visible within the transaction.
>
> The problem comes from process_owned_by() in sequence.c which has
> added in v10 some handling for internal dependencies in the case of an
> identity sequence, and the dependency link between the sequence and
> its relation is added there.
>
> Another thing I was wondering is if we should add the CCI directly to
> recordMultipleDependencies() for internal dependencies, still it seems
> to me that it would be an overkill as some dependency registrers may
> do the CCI by themselves after adding the pg_depend link and doing
> some other operations, so I discarded the idea.
>
> The patch attached solves the problem, for consistency I would suggest
> doing the CCI even for auto dependencies.

What is the general coding principle here?  "You need an CCI $WHEN"?

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15631: Generated as identity field in a temporary table with on commit drop corrupts system catalogs

Tom Lane-2
Peter Eisentraut <[hidden email]> writes:
> What is the general coding principle here?  "You need an CCI $WHEN"?

The general principle is "there should be a CCI after each independent
set of data/catalog changes".  You don't typically need CCI between
the primitive actions of a single DDL statement like CREATE SEQUENCE,
because you know that those actions are independent and don't look at
each others' output.  But you need one at the end, in case whatever
happens next should be able to see the results of the statement.

In another universe the rule might have been "CCI before each independent
set of actions", rather than after.  But we haven't done it that way.
Mixing those two styles as a method of fixing bugs seems like a horrid
idea: you eventually end up with fore-AND-aft CCIs everywhere, because
nobody knows what the preceding or following statements might've done.
For better or worse, the PG rule is CCI-after, and the stuff that does
DDL on identity sequences has to fall in line.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15631: Generated as identity field in a temporary table with on commit drop corrupts system catalogs

Peter Eisentraut-6
On 2019-02-15 15:43, Tom Lane wrote:
> Peter Eisentraut <[hidden email]> writes:
>> What is the general coding principle here?  "You need an CCI $WHEN"?
>
> The general principle is "there should be a CCI after each independent
> set of data/catalog changes".  You don't typically need CCI between
> the primitive actions of a single DDL statement like CREATE SEQUENCE,
> because you know that those actions are independent and don't look at
> each others' output.  But you need one at the end, in case whatever
> happens next should be able to see the results of the statement.

Yeah, I'm OK on that, but that's not really the problem here.  This is
not a case where a later step in a complex DDL command needs to see what
an earlier step did.  This is about that something later in the
transaction needs to see what happened earlier in the transaction.  This
does not seem to be the job of each individual DDL command; they don't
know what someone later might want to look at.  Otherwise many DDL
command implementations are lacking this CCI.  I think the CCI should be
more like at the end of ProcessUtility().

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15631: Generated as identity field in a temporary table with on commit drop corrupts system catalogs

Michael Paquier-2
On Fri, Feb 15, 2019 at 05:03:29PM +0100, Peter Eisentraut wrote:
> Yeah, I'm OK on that, but that's not really the problem here.  This is
> not a case where a later step in a complex DDL command needs to see what
> an earlier step did.  This is about that something later in the
> transaction needs to see what happened earlier in the transaction.  This
> does not seem to be the job of each individual DDL command; they don't
> know what someone later might want to look at.  Otherwise many DDL
> command implementations are lacking this CCI.  I think the CCI should be
> more like at the end of ProcessUtility().

Not all utility commands need a CCI, for example take VACUUM.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: BUG #15631: Generated as identity field in a temporary table with on commit drop corrupts system catalogs

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> On Fri, Feb 15, 2019 at 05:03:29PM +0100, Peter Eisentraut wrote:
>> Yeah, I'm OK on that, but that's not really the problem here.  This is
>> not a case where a later step in a complex DDL command needs to see what
>> an earlier step did.  This is about that something later in the
>> transaction needs to see what happened earlier in the transaction.  This
>> does not seem to be the job of each individual DDL command; they don't
>> know what someone later might want to look at.  Otherwise many DDL
>> command implementations are lacking this CCI.  I think the CCI should be
>> more like at the end of ProcessUtility().

> Not all utility commands need a CCI, for example take VACUUM.

BEGIN and COMMIT are more convincing counterexamples ...

                        regards, tom lane