BUG #14827: "ALTER TABLE... IF NOT EXISTS...ADD.. BIGSERIAL" leaves extra sequences

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

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

=?UTF-8?Q?Fabr=C3=ADzio_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

On Tue, Sep 26, 2017 at 4:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

=?UTF-8?Q?Fabr=C3=ADzio_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

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.

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

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