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.
>

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
Attachment
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.

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...


> 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... :-(


> We could maybe fix things by arranging to create the sequence only after
> 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,

--
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 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.​

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.
>  

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.
>

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
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