Обсуждение: Identity columns should own only one sequence
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
Вложения
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 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
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