Identity columns should own only one sequence

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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
/messages/by-id/3916586ef7f33948235fe60f54a3750046f5d940.camel@cybertec.at

So I have closed this commit fest item for now.

That's fine with me.

Yours,
Laurenz Albe

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

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