Thread: [BUGS] BUG #14827: "ALTER TABLE... IF NOT EXISTS...ADD.. BIGSERIAL" leavesextra sequences
[BUGS] BUG #14827: "ALTER TABLE... IF NOT EXISTS...ADD.. BIGSERIAL" leavesextra sequences
From
hvisage@gmail.com
Date:
The following bug has been logged on the website:
Bug reference: 14827
Logged by: Hendrik Visage
Email address: hvisage@gmail.com
PostgreSQL version: 9.6.5
Operating system: Debian 9
Description:
Goodday,With an alter table to add an UID field if it doesn't exist, the process
creates a sequence, but when the column does exist, the created sequence,
that isn't used, is just left there, and subsequent runs keeps adding extra
sequences.
postgres@tracsdbhvt01:~$ cat test-serial.sql
create database test;
\c test
create table test_serial ( teststring varchar(5));
alter table test_serial add column if not exists uid BIGSERIAL;
alter table test_serial add column if not exists uid BIGSERIAL;
\d
\d test_serial
postgres@tracsdbhvt01:~$ psql -p 5433 < test-serial.sql
CREATE DATABASE
You are now connected to database "test" as user "postgres".
CREATE TABLE
ALTER TABLE
NOTICE: column "uid" of relation "test_serial" already exists, skipping
ALTER TABLE List of relationsSchema | Name | Type | Owner
--------+----------------------+----------+----------public | test_serial | table | postgrespublic |
test_serial_uid_seq | sequence | postgrespublic | test_serial_uid_seq1 | sequence | postgres
(3 rows)
Table "public.test_serial" Column | Type |
Modifiers
------------+----------------------+-----------------------------------------------------------teststring | character
varying(5)|uid | bigint | not null default
nextval('test_serial_uid_seq'::regclass)
postgres@tracsdbhvt01:~$
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #14827: "ALTER TABLE... IF NOT EXISTS...ADD..BIGSERIAL" leaves extra sequences
From
Fabrízio de Royes Mello
Date:
On Mon, Sep 25, 2017 at 5:45 AM, <hvisage@gmail.com> wrote:
>
> The following bug has been logged on the website:
>
> Bug reference: 14827
> Logged by: Hendrik Visage
> Email address: hvisage@gmail.com
> PostgreSQL version: 9.6.5
> Operating system: Debian 9
> Description:
>
> Goodday,
> With an alter table to add an UID field if it doesn't exist, the process
> creates a sequence, but when the column does exist, the created sequence,
> that isn't used, is just left there, and subsequent runs keeps adding extra
> sequences.
>
> postgres@tracsdbhvt01:~$ cat test-serial.sql
> create database test;
> \c test
> create table test_serial ( teststring varchar(5));
> alter table test_serial add column if not exists uid BIGSERIAL;
> alter table test_serial add column if not exists uid BIGSERIAL;
> \d
> \d test_serial
> postgres@tracsdbhvt01:~$ psql -p 5433 < test-serial.sql
> CREATE DATABASE
> You are now connected to database "test" as user "postgres".
> CREATE TABLE
> ALTER TABLE
> NOTICE: column "uid" of relation "test_serial" already exists, skipping
> ALTER TABLE
> List of relations
> Schema | Name | Type | Owner
> --------+----------------------+----------+----------
> public | test_serial | table | postgres
> public | test_serial_uid_seq | sequence | postgres
> public | test_serial_uid_seq1 | sequence | postgres
> (3 rows)
>
> Table "public.test_serial"
> Column | Type | Modifiers
> ------------+----------------------+-----------------------------------------------------------
> teststring | character varying(5) |
> uid | bigint | not null default
> nextval('test_serial_uid_seq'::regclass)
>
> postgres@tracsdbhvt01:~$
>
>
That's awful... I'll take a look into it.
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
Re: [BUGS] BUG #14827: "ALTER TABLE... IF NOT EXISTS...ADD..BIGSERIAL" leaves extra sequences
From
Fabrízio de Royes Mello
Date:
On Tue, Sep 26, 2017 at 10:04 AM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>
> On Mon, Sep 25, 2017 at 5:45 AM, <hvisage@gmail.com> wrote:
> >
> > The following bug has been logged on the website:
> >
> > Bug reference: 14827
> > Logged by: Hendrik Visage
> > Email address: hvisage@gmail.com
> > PostgreSQL version: 9.6.5
> > Operating system: Debian 9
> > Description:
> >
> > Goodday,
> > With an alter table to add an UID field if it doesn't exist, the process
> > creates a sequence, but when the column does exist, the created sequence,
> > that isn't used, is just left there, and subsequent runs keeps adding extra
> > sequences.
> >
> > postgres@tracsdbhvt01:~$ cat test-serial.sql
> > create database test;
> > \c test
> > create table test_serial ( teststring varchar(5));
> > alter table test_serial add column if not exists uid BIGSERIAL;
> > alter table test_serial add column if not exists uid BIGSERIAL;
> > \d
> > \d test_serial
> > postgres@tracsdbhvt01:~$ psql -p 5433 < test-serial.sql
> > CREATE DATABASE
> > You are now connected to database "test" as user "postgres".
> > CREATE TABLE
> > ALTER TABLE
> > NOTICE: column "uid" of relation "test_serial" already exists, skipping
> > ALTER TABLE
> > List of relations
> > Schema | Name | Type | Owner
> > --------+----------------------+----------+----------
> > public | test_serial | table | postgres
> > public | test_serial_uid_seq | sequence | postgres
> > public | test_serial_uid_seq1 | sequence | postgres
> > (3 rows)
> >
> > Table "public.test_serial"
> > Column | Type | Modifiers
> > ------------+----------------------+-----------------------------------------------------------
> > teststring | character varying(5) |
> > uid | bigint | not null default
> > nextval('test_serial_uid_seq'::regclass)
> >
> > postgres@tracsdbhvt01:~$
> >
> >
>
> That's awful... I'll take a look into it.
>
>
> On Mon, Sep 25, 2017 at 5:45 AM, <hvisage@gmail.com> wrote:
> >
> > The following bug has been logged on the website:
> >
> > Bug reference: 14827
> > Logged by: Hendrik Visage
> > Email address: hvisage@gmail.com
> > PostgreSQL version: 9.6.5
> > Operating system: Debian 9
> > Description:
> >
> > Goodday,
> > With an alter table to add an UID field if it doesn't exist, the process
> > creates a sequence, but when the column does exist, the created sequence,
> > that isn't used, is just left there, and subsequent runs keeps adding extra
> > sequences.
> >
> > postgres@tracsdbhvt01:~$ cat test-serial.sql
> > create database test;
> > \c test
> > create table test_serial ( teststring varchar(5));
> > alter table test_serial add column if not exists uid BIGSERIAL;
> > alter table test_serial add column if not exists uid BIGSERIAL;
> > \d
> > \d test_serial
> > postgres@tracsdbhvt01:~$ psql -p 5433 < test-serial.sql
> > CREATE DATABASE
> > You are now connected to database "test" as user "postgres".
> > CREATE TABLE
> > ALTER TABLE
> > NOTICE: column "uid" of relation "test_serial" already exists, skipping
> > ALTER TABLE
> > List of relations
> > Schema | Name | Type | Owner
> > --------+----------------------+----------+----------
> > public | test_serial | table | postgres
> > public | test_serial_uid_seq | sequence | postgres
> > public | test_serial_uid_seq1 | sequence | postgres
> > (3 rows)
> >
> > Table "public.test_serial"
> > Column | Type | Modifiers
> > ------------+----------------------+-----------------------------------------------------------
> > teststring | character varying(5) |
> > uid | bigint | not null default
> > nextval('test_serial_uid_seq'::regclass)
> >
> > postgres@tracsdbhvt01:~$
> >
> >
>
> That's awful... I'll take a look into it.
>
I didn't came with better solution, but for now what I did is inside transformaAlterTableStmt when calling transformColumnDefinition now we pass down "AlterTableStmt->missing_ok" to check and skip CREATE SEQUENCE statements when use SERIAL pseudo-types.
It's not an elegant solution because during ATExecAddColumn we check it again by calling check_for_column_name_collision... Ideas are very welcome?
Att,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
Attachment
Re: [BUGS] BUG #14827: "ALTER TABLE... IF NOT EXISTS...ADD.. BIGSERIAL" leaves extra sequences
From
Tom Lane
Date:
Fabrízio de Royes Mello <fabriziomello@gmail.com> writes: > I didn't came with better solution, but for now what I did is inside > transformaAlterTableStmt when calling transformColumnDefinition now we pass > down "AlterTableStmt->missing_ok" to check and skip CREATE SEQUENCE > statements when use SERIAL pseudo-types. > It's not an elegant solution because during ATExecAddColumn we check it > again by calling check_for_column_name_collision... Ideas are very welcome? I do not think this is a solution at all. It doesn't address the fundamental problem that we decide whether to make a serial sequence before determining whether we're going to make a column default depending on it. What it does do is introduce a different set of failure conditions, basically race conditions around the discrepancy between parse-time and execution-time state. I don't feel like this is exactly a "must fix" problem, and it certainly isn't one that we should fix by introducing different oddities of behavior. We could maybe fix things by arranging to create the sequence only after we've made the column successfully, but that would be messy. I'm also not clear on how to document it. The documentation right now is quite clear, and accurate, about what SERIAL does: https://www.postgresql.org/docs/devel/static/datatype-numeric.html#datatype-serial This patch falsifies that, and so would any other conditional creation of the sequence. regards, tom lane -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] BUG #14827: "ALTER TABLE... IF NOT EXISTS...ADD..BIGSERIAL" leaves extra sequences
From
Fabrízio de Royes Mello
Date:
On Tue, Sep 26, 2017 at 4:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Fabrízio de Royes Mello <fabriziomello@gmail.com> writes:
> > I didn't came with better solution, but for now what I did is inside
> > transformaAlterTableStmt when calling transformColumnDefinition now we pass
> > down "AlterTableStmt->missing_ok" to check and skip CREATE SEQUENCE
> > statements when use SERIAL pseudo-types.
>
> > It's not an elegant solution because during ATExecAddColumn we check it
> > again by calling check_for_column_name_collision... Ideas are very welcome?
>
> I do not think this is a solution at all. It doesn't address the
> fundamental problem that we decide whether to make a serial sequence
> before determining whether we're going to make a column default
> depending on it.
>
> Fabrízio de Royes Mello <fabriziomello@gmail.com> writes:
> > I didn't came with better solution, but for now what I did is inside
> > transformaAlterTableStmt when calling transformColumnDefinition now we pass
> > down "AlterTableStmt->missing_ok" to check and skip CREATE SEQUENCE
> > statements when use SERIAL pseudo-types.
>
> > It's not an elegant solution because during ATExecAddColumn we check it
> > again by calling check_for_column_name_collision... Ideas are very welcome?
>
> I do not think this is a solution at all. It doesn't address the
> fundamental problem that we decide whether to make a serial sequence
> before determining whether we're going to make a column default
> depending on it.
I tried to address only the ALTER TABLE ... ADD COLUMN IF NOT EXISTS statement, and do not touch CREATE TABLE statements...
For example when we add a new SERIAL column to a relation:
ALTER TABLE foo ADD COLUMN bar SERIAL;
What I understood is actually PostgreSQL will convert it to:
1. CREATE SEQUENCE foo_bar_seq;
2. ALTER TABLE foo ADD COLUMN bar INTEGER DEFAULT nextval('foo_bar_seq');
3. ALTER SEQUENCE foo_bar_seq OWNER BY foo.bar;
And what I tried to implement is skip step 1 and 3... and fo step 2 skip the DEFAULT constraint
> What it does do is introduce a different set of failure
> conditions, basically race conditions around the discrepancy between
> parse-time and execution-time state.
>
I didn't understand what you mean here...
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
> conditions, basically race conditions around the discrepancy between
> parse-time and execution-time state.
>
I didn't understand what you mean here...
> I don't feel like this is exactly a "must fix" problem, and it certainly
> isn't one that we should fix by introducing different oddities of
> behavior.
>
> isn't one that we should fix by introducing different oddities of
> behavior.
>
When I see the code I felt the same... :-(
> We could maybe fix things by arranging to create the sequence only after
> we've made the column successfully, but that would be messy.
> we've made the column successfully, but that would be messy.
If we do that we should add more steps to execution queue...
> I'm also
> not clear on how to document it. The documentation right now is quite
> clear, and accurate, about what SERIAL does:
> https://www.postgresql.org/docs/devel/static/datatype-numeric.html#datatype-serial
> This patch falsifies that, and so would any other conditional creation
> of the sequence.
>
This patch doesn't falsifies that, because will act just when IF NOT EXISTS is used...
Regards,
> not clear on how to document it. The documentation right now is quite
> clear, and accurate, about what SERIAL does:
> https://www.postgresql.org/docs/devel/static/datatype-numeric.html#datatype-serial
> This patch falsifies that, and so would any other conditional creation
> of the sequence.
>
This patch doesn't falsifies that, because will act just when IF NOT EXISTS is used...
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
Re: [BUGS] BUG #14827: "ALTER TABLE... IF NOT EXISTS...ADD..BIGSERIAL" leaves extra sequences
From
"David G. Johnston"
Date:
On Tue, Sep 26, 2017 at 4:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:I tried to address only the ALTER TABLE ... ADD COLUMN IF NOT EXISTS statement, and do not touch CREATE TABLE statements...
And since our docs don't explain the equivalence in terms of ALTER TABLE we are not falsifying anything.
For example when we add a new SERIAL column to a relation:ALTER TABLE foo ADD COLUMN bar SERIAL;What I understood is actually PostgreSQL will convert it to:1. CREATE SEQUENCE foo_bar_seq;2. ALTER TABLE foo ADD COLUMN bar INTEGER DEFAULT nextval('foo_bar_seq');3. ALTER SEQUENCE foo_bar_seq OWNER BY foo.bar;
[...]
> I don't feel like this is exactly a "must fix" problem, and it certainly
> isn't one that we should fix by introducing different oddities of
> behavior.
>When I see the code I felt the same... :-(
Agreed, but it seems worthwhile to consider making it work as the OP expected.
> I'm also> not clear on how to document it. The documentation right now is quite
> clear, and accurate, about what SERIAL does:
> https://www.postgresql.org/docs/devel/static/datatype- numeric.html#datatype-serial
> This patch falsifies that, and so would any other conditional creation
> of the sequence.
>
This patch doesn't falsifies that, because will act just when IF NOT EXISTS is used...
And we already deviate for ALTER TABLE by not strictly adhering to the specified format: tablename_colname_seq; instead we allow for appending "N" to the end of the name if necessary to make the sequence name unique.
It seems like we'd want to invoke:
CREATE SEQUENCE IF NOT EXISTS tablename_colname_seq
If the corresponding add column is likewise IF NOT EXISTS.
If we detect the column was newly created maybe then also issue a RESET SEQUENCE just in case we picked up a left-over? This feels both cleaner and more dangerous than just inspecting everything first and deciding how to proceed on both fronts.
David J.
Re: [BUGS] BUG #14827: "ALTER TABLE... IF NOT EXISTS...ADD..BIGSERIAL" leaves extra sequences
From
Fabrízio de Royes Mello
Date:
On Tue, Sep 26, 2017 at 6:07 PM, David G. Johnston <david.g.johnston@gmail.com> wrote:
>
> On Tue, Sep 26, 2017 at 1:42 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>>
>> On Tue, Sep 26, 2017 at 4:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> I tried to address only the ALTER TABLE ... ADD COLUMN IF NOT EXISTS statement, and do not touch CREATE TABLE statements...
>
> And since our docs don't explain the equivalence in terms of ALTER TABLE we are not falsifying anything.
>
+1
>> For example when we add a new SERIAL column to a relation:
>>
>> ALTER TABLE foo ADD COLUMN bar SERIAL;
>>
>> What I understood is actually PostgreSQL will convert it to:
>>
>> 1. CREATE SEQUENCE foo_bar_seq;
>> 2. ALTER TABLE foo ADD COLUMN bar INTEGER DEFAULT nextval('foo_bar_seq');
>> 3. ALTER SEQUENCE foo_bar_seq OWNER BY foo.bar;
>>
> [...]
>
>>
>> > I don't feel like this is exactly a "must fix" problem, and it certainly
>> > isn't one that we should fix by introducing different oddities of
>> > behavior.
>> >
>>
>> When I see the code I felt the same... :-(
>
> Agreed, but it seems worthwhile to consider making it work as the OP expected.
>
>>
>> ALTER TABLE foo ADD COLUMN bar SERIAL;
>>
>> What I understood is actually PostgreSQL will convert it to:
>>
>> 1. CREATE SEQUENCE foo_bar_seq;
>> 2. ALTER TABLE foo ADD COLUMN bar INTEGER DEFAULT nextval('foo_bar_seq');
>> 3. ALTER SEQUENCE foo_bar_seq OWNER BY foo.bar;
>>
> [...]
>
>>
>> > I don't feel like this is exactly a "must fix" problem, and it certainly
>> > isn't one that we should fix by introducing different oddities of
>> > behavior.
>> >
>>
>> When I see the code I felt the same... :-(
>
> Agreed, but it seems worthwhile to consider making it work as the OP expected.
>
Sure... but the way things happen in code is not easy how to figure out a good elegant solution.
>> > I'm also
>> > not clear on how to document it. The documentation right now is quite
>> > clear, and accurate, about what SERIAL does:
>> > https://www.postgresql.org/docs/devel/static/datatype-numeric.html#datatype-serial
>> > This patch falsifies that, and so would any other conditional creation
>> > of the sequence.
>> >
>>
>> This patch doesn't falsifies that, because will act just when IF NOT EXISTS is used...
>>
>
> And we already deviate for ALTER TABLE by not strictly adhering to the specified format: tablename_colname_seq; instead we allow for appending "N" to the end of the name if necessary to make the sequence name unique.
>
> It seems like we'd want to invoke:
>
> CREATE SEQUENCE IF NOT EXISTS tablename_colname_seq
>
> If the corresponding add column is likewise IF NOT EXISTS.
>
> If we detect the column was newly created maybe then also issue a RESET SEQUENCE just in case we picked up a left-over? This feels both cleaner and more dangerous than just inspecting everything first and deciding how to proceed on both fronts.
>
>> > not clear on how to document it. The documentation right now is quite
>> > clear, and accurate, about what SERIAL does:
>> > https://www.postgresql.org/docs/devel/static/datatype-numeric.html#datatype-serial
>> > This patch falsifies that, and so would any other conditional creation
>> > of the sequence.
>> >
>>
>> This patch doesn't falsifies that, because will act just when IF NOT EXISTS is used...
>>
>
> And we already deviate for ALTER TABLE by not strictly adhering to the specified format: tablename_colname_seq; instead we allow for appending "N" to the end of the name if necessary to make the sequence name unique.
>
> It seems like we'd want to invoke:
>
> CREATE SEQUENCE IF NOT EXISTS tablename_colname_seq
>
> If the corresponding add column is likewise IF NOT EXISTS.
>
> If we detect the column was newly created maybe then also issue a RESET SEQUENCE just in case we picked up a left-over? This feels both cleaner and more dangerous than just inspecting everything first and deciding how to proceed on both fronts.
>
Seems a good plan... but I don't agree with RESET SEQUENCE... maybe just CREATE SEQUENCE IF NOT EXISTS when provide IF NOT EXISTS on ALTER TABLE ADD COLUMN is enough...
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
Re: [BUGS] BUG #14827: "ALTER TABLE... IF NOT EXISTS...ADD..BIGSERIAL" leaves extra sequences
From
Michael Paquier
Date:
On Wed, Sep 27, 2017 at 6:23 AM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > Seems a good plan... but I don't agree with RESET SEQUENCE... maybe just > CREATE SEQUENCE IF NOT EXISTS when provide IF NOT EXISTS on ALTER TABLE ADD > COLUMN is enough... Anything like that is not completely hole-proof either. Let's not forget that the sequence used with a default expression is not tracked with its name, so if the sequence created after the serial definition is renamed, and an INE is used on the given column, then you would still create a sequence. Even worse, we need to be careful about not linking the newly-created sequence instead of the one currently used. In my opinion, the current behavior is more predictible. I think that we should just document that IFE can leave behind sequences, and live with that. -- Michael -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs