Horton

Thread

Commits

  1. Make identity sequence management more robust

  1. Identity columns should own only one sequence

    Laurenz Albe <laurenz.albe@cybertec.at> — 2019-04-14T15:51:47Z

    Identity columns don't work if they own more than one sequence.

So if one tries to convert a "serial" column to an identity column,
the following can happen:

test=> CREATE TABLE ser(id serial);
CREATE TABLE
test=> ALTER TABLE ser ALTER id ADD GENERATED ALWAYS AS IDENTITY;
ERROR:  column "id" of relation "ser" already has a default value

Hm, ok, let's drop the column default value.

test=> ALTER TABLE ser ALTER id DROP DEFAULT;
ALTER TABLE

Now it works:

test=> ALTER TABLE ser ALTER id ADD GENERATED ALWAYS AS IDENTITY;
ALTER TABLE

But not very much:

test=> INSERT INTO ser (id) VALUES (DEFAULT);
ERROR:  more than one owned sequence found


I propose that we check if there already is a dependent sequence
before adding an identity column.

The attached patch does that, and also forbids setting the ownership
of a sequence to an identity column.

I think this should be backpatched.

Yours,
Laurenz Albe

  2. Re: Identity columns should own only one sequence

    Laurenz Albe <laurenz.albe@cybertec.at> — 2019-04-14T18:15:09Z

    I wrote:
> Identity columns don't work if they own more than one sequence.
> 
[...]
> test=> INSERT INTO ser (id) VALUES (DEFAULT);
> ERROR:  more than one owned sequence found
> 
> 
> I propose that we check if there already is a dependent sequence
> before adding an identity column.
> 
> The attached patch does that, and also forbids setting the ownership
> of a sequence to an identity column.

Alternatively, maybe getOwnedSequence should only consider sequences
with an "internal" dependency on the column.  That would avoid the problem
without forbidding anything, since normal OWNED BY dependencies are "auto".

What do you think?

Yours,
Laurenz Albe

  3. Re: Identity columns should own only one sequence

    Laurenz Albe <laurenz.albe@cybertec.at> — 2019-04-24T14:48:25Z

    On Sun, 2019-04-14 at 20:15 +0200, I wrote:
> I wrote:
> > Identity columns don't work if they own more than one sequence.
> 
> Alternatively, maybe getOwnedSequence should only consider sequences
> with an "internal" dependency on the column.  That would avoid the problem
> without forbidding anything, since normal OWNED BY dependencies are "auto".
> 
> What do you think?

Here is a patch that illustrates the second approach.

I'll add this thread to the next commitfest.

Yours,
Laurenz Albe

  4. Re: Identity columns should own only one sequence

    Michael Paquier <michael@paquier.xyz> — 2019-04-25T00:55:44Z

    On Sun, Apr 14, 2019 at 05:51:47PM +0200, Laurenz Albe wrote:
> test=> INSERT INTO ser (id) VALUES (DEFAULT);
> ERROR:  more than one owned sequence found

Yes this should never be user-triggerable, so it seems that we need to
fix and back-patch something if possible.

> I propose that we check if there already is a dependent sequence
> before adding an identity column.

That looks awkward.  Souldn't we make sure that when dropping the
default associated with a serial column then the dependency between
the column and the sequence is removed instead?  This implies more
complication in ATExecColumnDefault().

> The attached patch does that, and also forbids setting the ownership
> of a sequence to an identity column.
> 
> I think this should be backpatched.

Could you add some test cases with what you think is adapted?
--
Michael

  5. Re: Identity columns should own only one sequence

    Laurenz Albe <laurenz.albe@cybertec.at> — 2019-04-26T10:47:17Z

    On Thu, 2019-04-25 at 09:55 +0900, Michael Paquier wrote:
> On Sun, Apr 14, 2019 at 05:51:47PM +0200, Laurenz Albe wrote:
> > test=> INSERT INTO ser (id) VALUES (DEFAULT);
> > ERROR:  more than one owned sequence found
> 
> Yes this should never be user-triggerable, so it seems that we need to
> fix and back-patch something if possible.
> 
> > I propose that we check if there already is a dependent sequence
> > before adding an identity column.
> 
> That looks awkward.  Souldn't we make sure that when dropping the
> default associated with a serial column then the dependency between
> the column and the sequence is removed instead?  This implies more
> complication in ATExecColumnDefault().
> 
> > The attached patch does that, and also forbids setting the ownership
> > of a sequence to an identity column.
> > 
> > I think this should be backpatched.
> 
> Could you add some test cases with what you think is adapted?

You are right!  Dropping the dependency with the DEFAULT is the
cleanest approach.

I have left the checks to prevent double sequence ownership from
happening.

I added regression tests covering all added code.

Yours,
Laurenz Albe

  6. Re: Identity columns should own only one sequence

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-04-26T13:23:14Z

    On 2019-04-14 17:51, Laurenz Albe wrote:
> Identity columns don't work if they own more than one sequence.

Well, they shouldn't, because then how do they know which sequence they
should use?

> So if one tries to convert a "serial" column to an identity column,
> the following can happen:
> 
> test=> CREATE TABLE ser(id serial);
> CREATE TABLE
> test=> ALTER TABLE ser ALTER id ADD GENERATED ALWAYS AS IDENTITY;
> ERROR:  column "id" of relation "ser" already has a default value
> 
> Hm, ok, let's drop the column default value.
> 
> test=> ALTER TABLE ser ALTER id DROP DEFAULT;
> ALTER TABLE
> 
> Now it works:
> 
> test=> ALTER TABLE ser ALTER id ADD GENERATED ALWAYS AS IDENTITY;
> ALTER TABLE
> 
> But not very much:
> 
> test=> INSERT INTO ser (id) VALUES (DEFAULT);
> ERROR:  more than one owned sequence found

You also need to run

ALTER SEQUENCE ser_id_seq OWNED BY NONE;

because dropping the default doesn't release the linkage of the sequence
with the table.  These are just weird artifacts of how serial is
implemented, but that's why identity columns were added to improve
things.  I don't think we need to make things more complicated here.

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

  7. Re: Identity columns should own only one sequence

    Laurenz Albe <laurenz.albe@cybertec.at> — 2019-04-26T13:37:56Z

    On Fri, 2019-04-26 at 15:23 +0200, Peter Eisentraut wrote:
> > So if one tries to convert a "serial" column to an identity column,
> > the following can happen:
> > 
> > test=> CREATE TABLE ser(id serial);
> > CREATE TABLE
> > test=> ALTER TABLE ser ALTER id ADD GENERATED ALWAYS AS IDENTITY;
> > ERROR:  column "id" of relation "ser" already has a default value
> > 
> > Hm, ok, let's drop the column default value.
> > 
> > test=> ALTER TABLE ser ALTER id DROP DEFAULT;
> > ALTER TABLE
> > 
> > Now it works:
> > 
> > test=> ALTER TABLE ser ALTER id ADD GENERATED ALWAYS AS IDENTITY;
> > ALTER TABLE
> > 
> > But not very much:
> > 
> > test=> INSERT INTO ser (id) VALUES (DEFAULT);
> > ERROR:  more than one owned sequence found
> 
> You also need to run
> 
> ALTER SEQUENCE ser_id_seq OWNED BY NONE;
> 
> because dropping the default doesn't release the linkage of the sequence
> with the table.  These are just weird artifacts of how serial is
> implemented, but that's why identity columns were added to improve
> things.  I don't think we need to make things more complicated here.

What do you think of the patch I just posted on this thread to
remove ownership automatically when the default is dropped, as Michael
suggested?  I think that would make things much more intuitive from
the user's perspective.

Correct me if I am wrong, but the sequence behind identity columns
should be an implementation detail that the user doesn't have to know about.
So the error message about "owned sequences" is likely to confuse users.

I have had a report by a confused user, so I think the problem is real.

Yours,
Laurenz Albe

  8. Re: Identity columns should own only one sequence

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-04-26T15:55:34Z

    On 2019-Apr-26, Laurenz Albe wrote:

> What do you think of the patch I just posted on this thread to
> remove ownership automatically when the default is dropped, as Michael
> suggested?  I think that would make things much more intuitive from
> the user's perspective.

I think a better overall fix is that that when creating the generated
column (or altering a column to make it generated) we should look for
existing an existing sequence and take ownership of that (update
pg_depend records), before deciding to create a new sequence.

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

  9. Re: Identity columns should own only one sequence

    Michael Paquier <michael@paquier.xyz> — 2019-04-27T00:49:36Z

    On Fri, Apr 26, 2019 at 11:55:34AM -0400, Alvaro Herrera wrote:
> On 2019-Apr-26, Laurenz Albe wrote:
> I think a better overall fix is that that when creating the generated
> column (or altering a column to make it generated) we should look for
> existing an existing sequence and take ownership of that (update
> pg_depend records), before deciding to create a new sequence.

What that be actually right?  The current value of the sequence would
be the one from the previous use, and max/min values may not be the
defaults associated with identity columns, which is ranged in
[-2^31,2^31-1] by default, but the sequence you may attempt (or not)
to attach to could have completely different properties.  It seems to
me that it is much better to start afresh and not enforce the sequence
into something that the user may perhaps not want to use.

My 2c.
--
Michael

  10. Re: Identity columns should own only one sequence

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-04-27T12:16:01Z

    On 2019-04-26 15:37, Laurenz Albe wrote:
> What do you think of the patch I just posted on this thread to
> remove ownership automatically when the default is dropped, as Michael
> suggested?  I think that would make things much more intuitive from
> the user's perspective.

I think that adds more nonstandard behavior on top of an already
confusing and obsolescent feature, so I can't get too excited about it.

A more forward-looking fix would be your other idea of having
getOwnedSequence() only deal with identity sequences (not serial
sequences).  See attached patch for a draft.

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

  11. Re: Identity columns should own only one sequence

    Laurenz Albe <laurenz.albe@cybertec.at> — 2019-04-29T16:28:39Z

    On Sat, 2019-04-27 at 14:16 +0200, Peter Eisentraut wrote:
> On 2019-04-26 15:37, Laurenz Albe wrote:
> > What do you think of the patch I just posted on this thread to
> > remove ownership automatically when the default is dropped, as Michael
> > suggested?  I think that would make things much more intuitive from
> > the user's perspective.
> 
> I think that adds more nonstandard behavior on top of an already
> confusing and obsolescent feature, so I can't get too excited about it.
> 
> A more forward-looking fix would be your other idea of having
> getOwnedSequence() only deal with identity sequences (not serial
> sequences).  See attached patch for a draft.

That looks good to me.

I agree that slapping on black magic that appropriates a pre-existing
owned sequence seems out of proportion.

I still think thatthat there is merit to Michael's idea of removing
sequence "ownership" (which is just a dependency) when the DEFAULT
on the column is dropped, but this approach is possibly cleaner.

Yours,
Laurenz Albe

  12. Re: Identity columns should own only one sequence

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-05-02T20:43:47Z

    On 2019-04-29 18:28, Laurenz Albe wrote:
> I still think thatthat there is merit to Michael's idea of removing
> sequence "ownership" (which is just a dependency) when the DEFAULT
> on the column is dropped, but this approach is possibly cleaner.

I think the proper way to address this would be to create some kind of
dependency between the sequence and the default.

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

  13. Re: Identity columns should own only one sequence

    Laurenz Albe <laurenz.albe@cybertec.at> — 2019-05-03T06:14:35Z

    On Thu, 2019-05-02 at 22:43 +0200, Peter Eisentraut wrote:
> On 2019-04-29 18:28, Laurenz Albe wrote:
> > I still think thatthat there is merit to Michael's idea of removing
> > sequence "ownership" (which is just a dependency) when the DEFAULT
> > on the column is dropped, but this approach is possibly cleaner.
> 
> I think the proper way to address this would be to create some kind of
> dependency between the sequence and the default.

That is certainly true.  But that's hard to retrofit into existing databases,
so it would probably be a modification that is not backpatchable.

Yours,
Laurenz Albe

  14. Re: Identity columns should own only one sequence

    Michael Paquier <michael@paquier.xyz> — 2019-05-07T04:06:05Z

    On Fri, May 03, 2019 at 08:14:35AM +0200, Laurenz Albe wrote:
> On Thu, 2019-05-02 at 22:43 +0200, Peter Eisentraut wrote:
>> I think the proper way to address this would be to create some kind of
>> dependency between the sequence and the default.
> 
> That is certainly true.  But that's hard to retrofit into existing databases,
> so it would probably be a modification that is not backpatchable.

And this is basically already the dependency which exists between the
sequence and the relation created with the serial column.  So what's
the advantage of adding more dependencies if we already have what we
need?  I still think that we should be more careful to drop the
dependency between the sequence and the relation's column if dropping
the default using it.  If a DDL defines first a sequence, and then a
default expression using nextval() on a column, then no serial-related
dependency exist.
--
Michael

  15. Re: Identity columns should own only one sequence

    Laurenz Albe <laurenz.albe@cybertec.at> — 2019-05-08T14:49:23Z

    On Tue, 2019-05-07 at 13:06 +0900, Michael Paquier wrote:
> On Fri, May 03, 2019 at 08:14:35AM +0200, Laurenz Albe wrote:
> > On Thu, 2019-05-02 at 22:43 +0200, Peter Eisentraut wrote:
> >> I think the proper way to address this would be to create some kind of
> >> dependency between the sequence and the default.
> > 
> > That is certainly true.  But that's hard to retrofit into existing databases,
> > so it would probably be a modification that is not backpatchable.
> 
> And this is basically already the dependency which exists between the
> sequence and the relation created with the serial column.  So what's
> the advantage of adding more dependencies if we already have what we
> need?  I still think that we should be more careful to drop the
> dependency between the sequence and the relation's column if dropping
> the default using it.  If a DDL defines first a sequence, and then a
> default expression using nextval() on a column, then no serial-related

I believe we should have both:

- Identity columns should only use sequences with an INTERNAL dependency,
  as in Peter's patch.

- When a column default is dropped, remove all dependencies between the
  column and sequences.

In the spirit of moving this along, I have attached a patch which is
Peter's patch from above with a regression test.

Yours,
Laurenz Albe

  16. Re: Identity columns should own only one sequence

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-07-22T10:17:32Z

    On 2019-05-08 16:49, Laurenz Albe wrote:
> I believe we should have both:
> 
> - Identity columns should only use sequences with an INTERNAL dependency,
>   as in Peter's patch.

I have committed this.

> - When a column default is dropped, remove all dependencies between the
>   column and sequences.

There is no proposed patch for this, AFAICT.

So I have closed this commit fest item for now.

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

  17. Re: Identity columns should own only one sequence

    Laurenz Albe <laurenz.albe@cybertec.at> — 2019-08-05T11:24:32Z

    Peter Eisentraut wrote:
> On 2019-05-08 16:49, Laurenz Albe wrote:
> > I believe we should have both:
> > 
> > - Identity columns should only use sequences with an INTERNAL dependency,
> >   as in Peter's patch.
> 
> I have committed this.

Thanks!

> > - When a column default is dropped, remove all dependencies between the
> >   column and sequences.
> 
> There is no proposed patch for this, AFAICT.

There was one in
https://www.postgresql.org/message-id/3916586ef7f33948235fe60f54a3750046f5d940.camel%40cybertec.at

> So I have closed this commit fest item for now.

That's fine with me.

Yours,
Laurenz Albe

  18. Re: Identity columns should own only one sequence

    Laurenz Albe <laurenz.albe@cybertec.at> — 2019-08-05T11:30:43Z

    Peter Eisentraut wrote:
> On 2019-05-08 16:49, Laurenz Albe wrote:
> > I believe we should have both:
> > 
> > - Identity columns should only use sequences with an INTERNAL dependency,
> >   as in Peter's patch.
> 
> I have committed this.

Since this is a bug fix, shouldn't it be backpatched?

Yours,
Laurenz Albe

  19. Re: Identity columns should own only one sequence

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-08-06T09:54:14Z

    On 2019-08-05 13:30, Laurenz Albe wrote:
> Peter Eisentraut wrote:
>> On 2019-05-08 16:49, Laurenz Albe wrote:
>>> I believe we should have both:
>>>
>>> - Identity columns should only use sequences with an INTERNAL dependency,
>>>   as in Peter's patch.
>>
>> I have committed this.
> 
> Since this is a bug fix, shouldn't it be backpatched?

In cases where the workaround is "don't do that then", I'm inclined to
leave it alone.

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