Thread: identity columns
Here is another attempt to implement identity columns. This is a standard-conforming variant of PostgreSQL's serial columns. It also fixes a few usability issues that serial columns have: - need to set permissions on sequence in addition to table (*) - CREATE TABLE / LIKE copies default but refers to same sequence - cannot add/drop serialness with ALTER TABLE - dropping default does not drop sequence - slight weirdnesses because serial is some kind of special macro (*) Not actually implemented yet, because I wanted to make use of the NEXT VALUE FOR stuff I had previously posted, but I have more work to do there. There have been previous attempts at this between 2003 and 2007. The latest effort failed because it tried to implement identity columns and generated columns in one go, but then discovered that they have wildly different semantics. But AFAICT, there weren't any fundamental issues with the identity parts of the patch. Here some interesting threads of old: https://www.postgresql.org/message-id/flat/xzp1xw2x5jo.fsf%40dwp.des.no#xzp1xw2x5jo.fsf@dwp.des.no https://www.postgresql.org/message-id/flat/1146360362.839.104.camel%40home#1146360362.839.104.camel@home https://www.postgresql.org/message-id/23436.1149629297%40sss.pgh.pa.us https://www.postgresql.org/message-id/flat/448C9B7A.6010000%40dunaweb.hu#448C9B7A.6010000@dunaweb.hu https://www.postgresql.org/message-id/flat/45DACD1E.2000207%40dunaweb.hu#45DACD1E.2000207@dunaweb.hu https://www.postgresql.org/message-id/flat/18812.1178572575%40sss.pgh.pa.us Some comments on the implementation, and where it differs from previous patches: - The new attidentity column stores whether a column is an identity column and when it is generated (always/by default). I kept this independent from atthasdef mainly for he benefit of existing (client) code querying those catalogs, but I kept confusing myself by this, so I'm not sure about that choice. Basically we need to store four distinct states (nothing, default, identity always, identity by default) somehow. - The way attidentity is initialized in some places is a bit hackish. I haven't focused on that so much without having a clear resolution to the previous question. - One previous patch managed to make GENERATED an unreserved key word. I have it reserved right now, but perhaps with a bit more trying it can be unreserved. - I did not implement the restriction of one identity column per table. That didn't seem necessary. - I did not implement an override clause on COPY. If COPY supplies a value, it is always used. - pg_dump is as always a mess. Some of that is because of the way pg_upgrade works: It only gives out one OID at a time, so you need to create the table and sequences in separate entries. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Hello, The first look at the patch: On 8/30/16, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > Here is another attempt to implement identity columns. This is a > standard-conforming variant of PostgreSQL's serial columns. > > ... > > Some comments on the implementation, and where it differs from previous > patches: > > - The new attidentity column stores whether a column is an identity > column and when it is generated (always/by default). I kept this > independent from atthasdef mainly for he benefit of existing (client) > code querying those catalogs, but I kept confusing myself by this, so > I'm not sure about that choice. Basically we need to store four > distinct states (nothing, default, identity always, identity by default) > somehow. I don't have a string opinion which way is preferred. I think if the community is not against it, it can be left as is. > ... > - I did not implement the restriction of one identity column per table. > That didn't seem necessary. I think it should be mentioned in docs' "Compatibility" part as a PG's extension (similar to "Zero-column Tables"). > ... > > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services > Questions: 1. Is your patch implements T174 feature? Should a corresponding line be changed in src/backend/catalog/sql_features.txt? 2. Initializing attidentity in most places is ' ' but makefuncs.c has "n->identity = 0;". Is it correct? 3. doc/src/sgml/ref/create_table.sgml (5th chunk) has "TODO". Why? 4. There is "ADD GENERATED", but the standard says it should be "SET GENERATED" (ISO/IEC 9075-2 Subcl.11.20) 5. In ATExecAddIdentity: is it a good idea to check whether "attTup->attidentity" is the same as the given in "(ADD) SET GENERATED" and do nothing (except changing sequence's options) in addition to strict checking for "unset" (" ")? 6. In ATExecDropIdentity: is it a good idea to do nothing if the column is already not a identity (the same behavior as DROP NOT NULL/DROP DEFAULT)? 7. Is there any reason to insert CREATE_TABLE_LIKE_IDENTITY before CREATE_TABLE_LIKE_INDEXES, not at the end? Why do you change catversion.h? It can lead conflict when other patches influence it are committed... I'll have a closer look soon. -- Best regards, Vitaly Burovoy
Hello Peter,
I have reviewed the patch.
Currently it does not applies at the top of master, the last commit
without a conflict is 975768f
It compiles and passes "make check" tests, but fails with "make check-world" at:
test foreign_data ... FAILED
It tries to implement SQL:2011 feature T174 ("Identity columns"):
* column definition;
* column altering;
* inserting clauses "OVERRIDING {SYSTEM|USER} VALUE".
It has documentation changes.
===
The implementation has several distinctions from the standard:
1. The standard requires "... ALTER COLUMN ... SET GENERATED { ALWAYS
| BY DEFAULT }" (9075-2:2011 subcl 11.20), but the patch implements it
as "... ALTER COLUMN ... ADD GENERATED { ALWAYS | BY DEFAULT } AS
IDENTITY"
2. The standard requires not more than one identity column, the patch
does not follow that requirement, but it does not mentioned in the
doc.
3. Changes in the table "information_schema.columns" is not full. Some
required columns still empty for identity columns:
* identity_start
* identity_increment
* identity_maximum
* identity_minimum
* identity_cycle
4. "<alter identity column specification>" is not fully implemented
because "<set identity column generation clause>" is implemented
whereas "<alter identity column option>" is not.
5. According to 9075-2:2011 subcl 14.11 Syntax Rule 11)c) for a column
with an indication that values are generated by default the only
possible "<override clause>" is "OVERRIDING USER VALUE".
Implementation allows to use "OVERRIDING SYSTEM VALUE" (as "do
nothing"), but it should be mentioned in "Compatibility" part in the
doc.
postgres=# CREATE TABLE itest10 (a int generated BY DEFAULT as
identity PRIMARY KEY, b text);
CREATE TABLE
postgres=# INSERT INTO itest10 overriding SYSTEM value SELECT 10, 'a';
INSERT 0 1
postgres=# SELECT * FROM itest10;a | b
---+---10 | a
(1 row)
===
6. "CREATE TABLE ... (LIKE ... INCLUDING ALL)" fails Assertion at
src/backend/commands/tablecmds.c:631
postgres=# CREATE TABLE itest1 (a int generated by default as identity, b text);
CREATE TABLE
postgres=# CREATE TABLE x(LIKE itest1 INCLUDING ALL);
server closed the connection unexpectedly This probably means the server terminated abnormally before or
whileprocessing the request.
The connection to the server was lost. Attempting reset: Failed.
===
Also the implementation has several flaws in corner cases:
7. Changing default is allowed but a column is still "identity":
postgres=# CREATE TABLE itest4 (a int GENERATED ALWAYS AS IDENTITY, b text);
CREATE TABLE
postgres=# ALTER TABLE itest4 ALTER COLUMN a set DEFAULT 1;
ALTER TABLE
postgres=# \d itest4 Table "public.itest4"Column | Type | Modifiers
--------+---------+--------------------------------------------------a | integer | not null default 1 generated
alwaysas identityb | text |
---
8. Changing a column to be "identity" raises "duplicate key" exception:
postgres=# CREATE TABLE itest4 (a serial, b text);
CREATE TABLE
postgres=# ALTER TABLE itest4 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
ERROR: duplicate key value violates unique constraint
"pg_attrdef_adrelid_adnum_index"
---
9. Changing type of a column deletes linked sequence but leaves
"default" and "identity" marks:
postgres=# CREATE TABLE itest4 (a int GENERATED ALWAYS AS IDENTITY, b int);
CREATE TABLE
postgres=# ALTER TABLE itest4 ALTER COLUMN a TYPE text;
ALTER TABLE
postgres=# \d itest4; Table "public.itest4"Column | Type |
Modifiers
--------+---------+------------------------------------------------------------------a | text | default
nextval('16445'::regclass) generated
always as identityb | integer |
postgres=# insert into itest4(b) values(1);
ERROR: could not open relation with OID 16445
postgres=# select * from itest4;a | b
---+---
(0 rows)
---
10. "identity" modifier is lost when the table inherits another one:
postgres=# CREATE TABLE itest_err_1 (a int);
CREATE TABLE
postgres=# CREATE TABLE x (a int GENERATED ALWAYS AS
IDENTITY)inherits(itest_err_1);
NOTICE: merging column "a" with inherited definition
CREATE TABLE
postgres=# \d itest_err_1; \d x; Table "public.itest_err_1"Column | Type | Modifiers
--------+---------+-----------a | integer |
Number of child tables: 1 (Use \d+ to list them.)
Table "public.x"Column | Type | Modifiers
--------+---------+-----------------------------------------------a | integer | not null default
nextval('x_a_seq'::regclass)
Inherits: itest_err_1
---
11. The documentation says "OVERRIDING ... VALUE" can be placed even
before "DEFAULT VALUES", but it is against SQL spec and the
implementation:
postgres=# CREATE TABLE itest10 (a int GENERATED BY DEFAULT AS
IDENTITY, b text);
CREATE TABLE
postgres=# INSERT INTO itest10 DEFAULT VALUES;
INSERT 0 1
postgres=# INSERT INTO itest10 OVERRIDING USER VALUE VALUES(1,2);
INSERT 0 1
postgres=# INSERT INTO itest10 OVERRIDING USER VALUE DEFAULT VALUES;
ERROR: syntax error at or near "DEFAULT" at character 43
---
12. Dump/restore is broken for some cases:
postgres=# CREATE SEQUENCE itest1_a_seq;
CREATE SEQUENCE
postgres=# CREATE TABLE itest1 (a int generated by default as identity, b text);
CREATE TABLE
postgres=# DROP SEQUENCE itest1_a_seq;
DROP SEQUENCE
postgres=# CREATE DATABASE a;
CREATE DATABASE
postgres=# \q
comp ~ $ pg_dump postgres | psql a
SET
SET
SET
SET
SET
SET
SET
SET
COMMENT
CREATE EXTENSION
COMMENT
SET
SET
SET
CREATE TABLE
ALTER TABLE
ALTER TABLE
COPY 0
ERROR: relation "itest1_a_seq1" does not exist
LINE 1: SELECT pg_catalog.setval('itest1_a_seq1', 2, true);
---
13. doc/src/sgml/ref/create_table.sgml (5th chunk) has "TODO". Why?
---
14. It would be fine if psql has support of new clauses.
===
Also several notes:
15. Initializing attidentity in most places is ' ' but makefuncs.c has
"n->identity = 0;". Is it correct?
---
16. I think it is a good idea to not raise exceptions for "SET
GENERATED/DROP IDENTITY" if a column has the same type of identity/not
an identity. To be consistent with "SET/DROP NOT NULL".
--
Best regards,
Vitaly Burovoy
Thank you for this extensive testing. I will work on getting the bugs
fixed. Just a couple of comments on some of your points:
On 9/9/16 11:45 PM, Vitaly Burovoy wrote:
> It compiles and passes "make check" tests, but fails with "make check-world" at:
> test foreign_data ... FAILED
I do not see that. You can you show the diffs?
> 1. The standard requires "... ALTER COLUMN ... SET GENERATED { ALWAYS
> | BY DEFAULT }" (9075-2:2011 subcl 11.20), but the patch implements it
> as "... ALTER COLUMN ... ADD GENERATED { ALWAYS | BY DEFAULT } AS
> IDENTITY"
SET and ADD are two different things. The SET command just changes the
parameters of the underlying sequence. This can be implemented later
and doesn't seem so important now. The ADD command is not in the
standard, but I needed it for pg_dump, mainly. I will need to document
this.
> 14. It would be fine if psql has support of new clauses.
What do you mean by that? Tab completion?
> 16. I think it is a good idea to not raise exceptions for "SET
> GENERATED/DROP IDENTITY" if a column has the same type of identity/not
> an identity. To be consistent with "SET/DROP NOT NULL".
These behaviors are per SQL standard.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 9/12/16, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> Thank you for this extensive testing. I will work on getting the bugs
> fixed. Just a couple of comments on some of your points:
>
> On 9/9/16 11:45 PM, Vitaly Burovoy wrote:
>> It compiles and passes "make check" tests, but fails with "make
>> check-world" at:
>> test foreign_data ... FAILED
>
> I do not see that. You can you show the diffs?
I can't reproduce it, it is my fault, may be I did not clean build dir.
>> 1. The standard requires "... ALTER COLUMN ... SET GENERATED { ALWAYS
>> | BY DEFAULT }" (9075-2:2011 subcl 11.20), but the patch implements it
>> as "... ALTER COLUMN ... ADD GENERATED { ALWAYS | BY DEFAULT } AS
>> IDENTITY"
>
> SET and ADD are two different things. The SET command just changes the
> parameters of the underlying sequence.
Well... As for me ADD is used when you can add more than one property
of the same kind to a relation (e.g. column or constraint), but SET is
used when you change something and it replaces previous state (e.g.
NOT NULL, DEFAULT, STORAGE, SCHEMA, TABLESPACE etc.)
You can't set ADD more than one IDENTITY to a column, so it should be "SET".
> This can be implemented later and doesn't seem so important now.
Hmm. Now you're passing params to CreateSeqStmt because they are the same.
Is it hard to pass them to AlterSeqStmt (if there is no SET GENERATED")?
> The ADD command is not in the standard, but I needed it for pg_dump, mainly.
> I will need to document this.
Firstly, why to introduce new grammar which differs from the standard
instead of follow the standard?
Secondly, I see no troubles to follow the standard:
src/bin/pg_dump/pg_dump.c:
- "ALTER COLUMN %s ADD GENERATED ",
+ "ALTER COLUMN %s SET GENERATED ",
src/backend/parser/gram.y:
- /* ALTER TABLE <name> ALTER [COLUMN] <colname> ADD GENERATED ... AS
IDENTITY ... */
- | ALTER opt_column ColId ADD_P GENERATED generated_when AS
IDENTITY_P OptParenthesizedSeqOptList
- c->options = $9;
+ /* ALTER TABLE <name> ALTER [COLUMN] <colname> SET GENERATED ... */
+ | ALTER opt_column ColId SET GENERATED generated_when OptSeqOptList
- c->options = $7;
I guess "ALTER opt_column ColId SET OptSeqOptList" is easy to be
implemented, after some research "ALTER opt_column ColId RESTART [WITH
...]" also can be added.
(and reflected in the docs)
>> 14. It would be fine if psql has support of new clauses.
>
> What do you mean by that? Tab completion?
Yes, I'm about it. Or tab completion is usually developed later?
>> 16. I think it is a good idea to not raise exceptions for "SET
>> GENERATED/DROP IDENTITY" if a column has the same type of identity/not
>> an identity. To be consistent with "SET/DROP NOT NULL".
>
> These behaviors are per SQL standard.
Can you point to concrete rule(s) in the standard?
I could not find it in ISO/IEC 9075-2 subclauses 11.20 "<alter
identity column specification>" and 11.21 "<drop identity property
clause>".
Only subclause 4.15.11 "Identity columns" says "The columns of a base
table BT can optionally include not more than one identity column."
(which you don't follow).
For instance, subclause 11.42 <drop character set statement>, General
Rules p.1 says explicitly about exception.
Or (for columns): 11.4 <column definition>, General Rules p.3: "The
<column name> in the <column definition> SHALL NOT be equivalent to
the <column name> of any other column of T."
> --
> Peter Eisentraut http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Several additional thoughts:
1. I think it is wise to add ability to set name of a sequence (as
PG's extension of the standard) to SET GENERATED or GENERATED in a
relation definition (something like CONSTRAINTs names), without it it
is hard to fix conflicts with other sequences (e.g. from serial pseudo
type) and manual changes of the sequence ("alter seq rename", "alter
seq set schema" etc.).
2. Is it useful to rename sequence linked with identity constraint
when table is renamed (similar way when sequence moves to another
schema following the linked table)?
3. You're setting OWNER to a sequence, but what about USAGE privilege
to roles have INSERT/UPDATE privileges to the table? For security
reasons table is owned by a role different from roles which are using
the table (to prevent changing its definition).
--
Best regards,
Vitaly Burovoy
On Mon, Sep 12, 2016 at 5:02 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > Thank you for this extensive testing. I will work on getting the bugs > fixed. It looks like the patch has not been updated; since the CommitFest is (hopefully) wrapping up, I am marking this "Returned with Feedback" for now. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
New patch.
On 9/9/16 11:45 PM, Vitaly Burovoy wrote:
> 1. The standard requires "... ALTER COLUMN ... SET GENERATED { ALWAYS
> | BY DEFAULT }" (9075-2:2011 subcl 11.20), but the patch implements it
> as "... ALTER COLUMN ... ADD GENERATED { ALWAYS | BY DEFAULT } AS
> IDENTITY"
Has both now. They do different things, as documented.
> 2. The standard requires not more than one identity column, the patch
> does not follow that requirement, but it does not mentioned in the
> doc.
fixed
> 3. Changes in the table "information_schema.columns" is not full.
fixed
> 4. "<alter identity column specification>" is not fully implemented
> because "<set identity column generation clause>" is implemented
> whereas "<alter identity column option>" is not.
done
> 5. According to 9075-2:2011 subcl 14.11 Syntax Rule 11)c) for a column
> with an indication that values are generated by default the only
> possible "<override clause>" is "OVERRIDING USER VALUE".
> Implementation allows to use "OVERRIDING SYSTEM VALUE" (as "do
> nothing"), but it should be mentioned in "Compatibility" part in the
> doc.
done (documented)
> 6. "CREATE TABLE ... (LIKE ... INCLUDING ALL)" fails Assertion at
> src/backend/commands/tablecmds.c:631
fixed
> 7. Changing default is allowed but a column is still "identity":
fixed
> 8. Changing a column to be "identity" raises "duplicate key" exception:
fixed
> 9. Changing type of a column deletes linked sequence but leaves
> "default" and "identity" marks:
fixed
> 10. "identity" modifier is lost when the table inherits another one:
fixed, but I invite more testing of inheritance-related things
> 11. The documentation says "OVERRIDING ... VALUE" can be placed even
> before "DEFAULT VALUES", but it is against SQL spec and the
> implementation:
fixed
> 12. Dump/restore is broken for some cases:
fixed
> 13. doc/src/sgml/ref/create_table.sgml (5th chunk) has "TODO". Why?
fixed
> 14. It would be fine if psql has support of new clauses.
done
> 15. Initializing attidentity in most places is ' ' but makefuncs.c has
> "n->identity = 0;". Is it correct?
fixed
> 16. I think it is a good idea to not raise exceptions for "SET
> GENERATED/DROP IDENTITY" if a column has the same type of identity/not
> an identity. To be consistent with "SET/DROP NOT NULL".
The present behavior is per SQL standard.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Hi Vitaly,
This is a gentle reminder.
you assigned as reviewer to the current patch in the 11-2016 commitfest.
But you haven't shared your review yet on the new patch in this commitfest.
Please share your review about the patch. This will help us in smoother
operation of commitfest.
Please Ignore if you already shared your review.
Regards,
Hari Babu
Fujitsu Australia
On Tue, Nov 22, 2016 at 10:51 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
Hi Vitaly,This is a gentle reminder.you assigned as reviewer to the current patch in the 11-2016 commitfest.But you haven't shared your review yet on the new patch in this commitfest.Please share your review about the patch. This will help us in smootheroperation of commitfest.Please Ignore if you already shared your review.
Moved to next CF with "needs review" status.
Regards,
Hari Babu
Fujitsu Australia
Updated patch with some merge conflicts resolved and some minor bug fixes. Vitaly's earlier reviews were very comprehensive, and I believe I have fixed all the issues that have been pointed out, so just double-checking that would be helpful at this point. I still don't have a solution for managing access to the implicit sequences without permission checking, but I have an idea, so I might send an update sometime. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Hello, Peter,
I apologize for a silence since the last CF.
I've tested your last patch and have several nitpickings:
1. The fact COPY ignores GENERATED ALWAYS constraint (treats as
GENERATED BY DEFAULT) should be mentioned as well as rules.
2. Usually error message for identical columns (with LIKE clause)
looks like this:
test=# CREATE TABLE def(i serial, n1 int NOT NULL, n2 int);
CREATE TABLE
test=# CREATE TABLE test(i serial, LIKE def INCLUDING ALL);
ERROR: column "i" specified more than once
but for generated columns it is disorienting enough:
test=# CREATE TABLE idnt(i int GENERATED BY DEFAULT AS IDENTITY);
CREATE TABLE
test=# CREATE TABLE test(i int GENERATED BY DEFAULT AS IDENTITY, LIKE
idnt INCLUDING ALL);
ERROR: relation "test_i_seq" already exists
3. Strange error (not about absence of a column; but see pp.5 and 8):
test=# ALTER TABLE idnt ALTER COLUMN o ADD GENERATED ALWAYS AS IDENTITY;
ERROR: identity column type must be smallint, integer, or bigint
4. Altering type leads to an error:
test=# ALTER TABLE idnt ALTER COLUMN i TYPE bigint;
ERROR: cannot alter data type of identity column "i"
it is understandable for non-integers. But why do you forbid changing
to the supported types?
5. Attempt to make a column be identity fails:
test=# ALTER TABLE idnt ALTER COLUMN n1 SET GENERATED BY DEFAULT;
ERROR: column "n1" of relation "idnt" is not an identity column
As I understand from the Spec, chapter 11.20 General Rule 2)b) says
about the final state of a column without mentioning of the initial
state.
Therefore even if the column has the initial state "not generated",
after the command it changes the state to either "GENERATED ALWAYS" or
"GENERATED BY DEFAULT".
6. The docs mention a syntax:
ALTER [ COLUMN ] <replaceable
class="PARAMETER">column_name</replaceable> { SET GENERATED { ALWAYS |
BY DEFAULT } | SET <replaceable>sequence_option</replaceable> | RESET
} [...]
but the command fails:
test=# ALTER TABLE idnt ALTER COLUMN i RESET;
ERROR: syntax error at or near ";"
LINE 1: ALTER TABLE idnt ALTER COLUMN i RESET;
7. (the same line of the doc)
usually ellipsis appears in inside parenthesis with clauses can be
repeated, in other words it should be written as:
ALTER <skipped> column_name</replaceable> ( { SET GENERATED { ALWAYS |
BY DEFAULT } | <skipped> } [...] )
8. To avoid unnecessary syntax "ALTER COLUMN ... ADD GENERATED ..."
and use existing syntax "ALTER COLUMN ... SET GENERATED ..." you can
just make the "SET" word be optional as a PG's extension. I.e. instead
of:
test=# ALTER TABLE idnt ALTER i SET GENERATED ALWAYS SET INCREMENT BY 4;
you can write:
test=# ALTER TABLE idnt ALTER i SET GENERATED ALWAYS INCREMENT BY 4;
test=# ALTER TABLE idnt ALTER n1 SET GENERATED ALWAYS INCREMENT BY 4;
-- which sets identity constraint - see p.5
which is very similar to your extended syntax:
test=# ALTER TABLE idnt ALTER n1 ADD GENERATED ALWAYS AS IDENTITY
(INCREMENT BY 4);
9. The command fails:
test=# ALTER TABLE idnt ALTER n2 ADD GENERATED ALWAYS AS IDENTITY;
ERROR: column "n2" of relation "idnt" must be declared NOT NULL
before identity can be added
whereas the Spec does not contains a requirement for a column to be a
NOT NULLABLE.
You can implicitly set a column as NOT NULL (as the "serial" macros
does), but not require it later.
Moreover you can get a NULLABLE identity column by:
test=# ALTER TABLE idnt ALTER COLUMN i DROP NOT NULL;
ALTER TABLE
test=# \d idnt Table "public.idnt"Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+------------------------------i | integer | | |
generatedalways as identityn1 | integer | | not null |n2 | integer | | |
10. Inherited tables are not allowed at all (even with explicit
identity columns):
test=# CREATE TABLE inh() INHERITS (idnt);
ERROR: cannot inherit from table with identity column
test=# CREATE TABLE inh(i int GENERATED BY DEFAULT AS IDENTITY) INHERITS (idnt);
ERROR: cannot inherit from table with identity column
What's the point of forbidding tables inherited from one with identity column?
It makes identity columns be useless for big tables.
Just declare identity constraint as non-inherited (as well as some
other constraints (which identity is) - PK, FK, UNIQUE)...
Also see p.11
11. The last CF added partitioned tables which are similar to
inherited, but slightly different.
Slightly changed example from [1] (identity column added):
test=# CREATE TABLE measurement (
test(# i int GENERATED BY DEFAULT AS IDENTITY,
test(# logdate date not null,
test(# peaktemp int,
test(# unitsales int
test(# ) PARTITION BY RANGE (logdate);
CREATE TABLE
test=# CREATE TABLE measurement_y2016m07
test-# PARTITION OF measurement (
test(# unitsales DEFAULT 0
test(# ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
ERROR: cannot inherit from table with identity column
test=# INSERT INTO measurement(logdate) VALUES('2016-07-10');
ERROR: no partition of relation "measurement" found for row
DETAIL: Failing row contains (1, 2016-07-10, null, null).
12. You've introduced a new parameter for a sequence declaration
("SEQUENCE NAME") which is not mentioned in the docs and not
supported:
a2=# CREATE SEQUENCE s SEQUENCE NAME s;
ERROR: option "sequence_name" not recognized
I think the error should look as:
a2=# CREATE SEQUENCE s SEQUENCE__ NAME s;
ERROR: syntax error at or near "SEQUENCE__"
LINE 1: CREATE SEQUENCE s SEQUENCE__ NAME s; ^
13. Also strange error message:
test=# CREATE SCHEMA sch;
test=# ALTER TABLE idnt ALTER COLUMN n1 ADD GENERATED BY DEFAULT AS
IDENTITY (SEQUENCE NAME sch.s START 1);
ERROR: relation "sch.idnt" does not exist
But if a table sch.idnt exists, it leads to a silent inconsistency:
test=# CREATE TABLE sch.idnt(n1 int);
CREATE TABLE
test=# ALTER TABLE idnt ALTER COLUMN n1 ADD GENERATED BY DEFAULT AS
IDENTITY (SEQUENCE NAME sch.s START 1);
ALTER TABLE
test=# DROP TABLE sch.idnt;
ERROR: could not find tuple for attrdef 0
Also dump/restore fails for it.
Note that by default sequences have different error message:
test=# CREATE SEQUENCE sch.s1 OWNED BY idnt.n1;
ERROR: sequence must be in same schema as table it is linked to
14. Wrong hint message:
test=# DROP SEQUENCE idnt_i_seq;
ERROR: cannot drop sequence idnt_i_seq because default for table idnt
column i requires it
HINT: You can drop default for table idnt column i instead.
but according to DDL there is no "default" which can be dropped:
test=# ALTER TABLE idnt ALTER COLUMN i DROP DEFAULT;
ERROR: column "i" of relation "idnt" is an identity column
15. And finally. The command:
test=# CREATE TABLE t(i int GENERATED BY DEFAULT AS IDENTITY (SEQUENCE NAME s));
leads to a core dump.
It happens when no sequence parameter (like "START") is set.
[1]https://www.postgresql.org/docs/devel/static/sql-createtable.html
--
Best regards,
Vitaly Burovoy
On Thu, Jan 5, 2017 at 9:34 AM, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote: > I apologize for a silence since the last CF. > I've tested your last patch and have several nitpickings: Last update is a review from 3 weeks ago, this patch is marked as returned with feedback. -- Michael
New patch that fixes everything. ;-) Besides hopefully addressing all
your issues, this version also no longer requires separate privileges on
the internal sequence, which was the last outstanding functional issue
that I am aware of. This version also no longer stores a default entry
in pg_attrdef. Instead, the rewriter makes up the default expression on
the fly. This makes some things much simpler.
On 1/4/17 19:34, Vitaly Burovoy wrote:
> 1. The fact COPY ignores GENERATED ALWAYS constraint (treats as
> GENERATED BY DEFAULT) should be mentioned as well as rules.
fixed (documentation added)
What do you mean for rules?
> 2. Usually error message for identical columns (with LIKE clause)
> looks like this:
> test=# CREATE TABLE def(i serial, n1 int NOT NULL, n2 int);
> CREATE TABLE
> test=# CREATE TABLE test(i serial, LIKE def INCLUDING ALL);
> ERROR: column "i" specified more than once
>
> but for generated columns it is disorienting enough:
> test=# CREATE TABLE idnt(i int GENERATED BY DEFAULT AS IDENTITY);
> CREATE TABLE
> test=# CREATE TABLE test(i int GENERATED BY DEFAULT AS IDENTITY, LIKE
> idnt INCLUDING ALL);
> ERROR: relation "test_i_seq" already exists
Yeah, this is kind of hard to fix though, because the sequence names
are decided during parse analysis, when we don't see that the same
sequence name is also used by another new column. We could maybe
patch around it in an ugly way, but it doesn't seem worth it for this
marginal case.
> 3. Strange error (not about absence of a column; but see pp.5 and 8):
> test=# ALTER TABLE idnt ALTER COLUMN o ADD GENERATED ALWAYS AS IDENTITY;
> ERROR: identity column type must be smallint, integer, or bigint
What's wrong with that?
> 4. Altering type leads to an error:
> test=# ALTER TABLE idnt ALTER COLUMN i TYPE bigint;
> ERROR: cannot alter data type of identity column "i"
>
> it is understandable for non-integers. But why do you forbid changing
> to the supported types?
fixed (is now allowed)
> 5. Attempt to make a column be identity fails:
> test=# ALTER TABLE idnt ALTER COLUMN n1 SET GENERATED BY DEFAULT;
> ERROR: column "n1" of relation "idnt" is not an identity column
>
> As I understand from the Spec, chapter 11.20 General Rule 2)b) says
> about the final state of a column without mentioning of the initial
> state.
> Therefore even if the column has the initial state "not generated",
> after the command it changes the state to either "GENERATED ALWAYS" or
> "GENERATED BY DEFAULT".
11.12 <alter column definition> states that "If <alter identity column
specification> or <drop identity property clause> is specified, then C
shall be an identity column." So this clause is only to alter an
existing identity column, not make a new one.
> 6. The docs mention a syntax:
> ALTER [ COLUMN ] <replaceable
> class="PARAMETER">column_name</replaceable> { SET GENERATED { ALWAYS |
> BY DEFAULT } | SET <replaceable>sequence_option</replaceable> | RESET
> } [...]
>
> but the command fails:
> test=# ALTER TABLE idnt ALTER COLUMN i RESET;
> ERROR: syntax error at or near ";"
> LINE 1: ALTER TABLE idnt ALTER COLUMN i RESET;
This works for me. Check again please.
> 7. (the same line of the doc)
> usually ellipsis appears in inside parenthesis with clauses can be
> repeated, in other words it should be written as:
> ALTER <skipped> column_name</replaceable> ( { SET GENERATED { ALWAYS |
> BY DEFAULT } | <skipped> } [...] )
But there are no parentheses in that syntax. I think the syntax
synopsis as written is correct.
> 8. To avoid unnecessary syntax "ALTER COLUMN ... ADD GENERATED ..."
> and use existing syntax "ALTER COLUMN ... SET GENERATED ..." you can
> just make the "SET" word be optional as a PG's extension. I.e. instead
> of:
> test=# ALTER TABLE idnt ALTER i SET GENERATED ALWAYS SET INCREMENT BY 4;
>
> you can write:
> test=# ALTER TABLE idnt ALTER i SET GENERATED ALWAYS INCREMENT BY 4;
> test=# ALTER TABLE idnt ALTER n1 SET GENERATED ALWAYS INCREMENT BY 4;
> -- which sets identity constraint - see p.5
>
> which is very similar to your extended syntax:
> test=# ALTER TABLE idnt ALTER n1 ADD GENERATED ALWAYS AS IDENTITY
> (INCREMENT BY 4);
See under 5 that that is not correct.
> 9. The command fails:
> test=# ALTER TABLE idnt ALTER n2 ADD GENERATED ALWAYS AS IDENTITY;
> ERROR: column "n2" of relation "idnt" must be declared NOT NULL
> before identity can be added
>
> whereas the Spec does not contains a requirement for a column to be a
> NOT NULLABLE.
> You can implicitly set a column as NOT NULL (as the "serial" macros
> does), but not require it later.
The spec requires that an identity column is NOT NULL.
> Moreover you can get a NULLABLE identity column by:
> test=# ALTER TABLE idnt ALTER COLUMN i DROP NOT NULL;
> ALTER TABLE
> test=# \d idnt
> Table "public.idnt"
> Column | Type | Collation | Nullable | Default
> --------+---------+-----------+----------+------------------------------
> i | integer | | | generated always as identity
> n1 | integer | | not null |
> n2 | integer | | |
Fixed by disallowing that command (similar to dropping NOT NULL from a
primary key, for example).
> 10. Inherited tables are not allowed at all
fixed
> 11. The last CF added partitioned tables which are similar to
> inherited, but slightly different.
fixed
> 12. You've introduced a new parameter for a sequence declaration
> ("SEQUENCE NAME") which is not mentioned in the docs and not
> supported:
> a2=# CREATE SEQUENCE s SEQUENCE NAME s;
> ERROR: option "sequence_name" not recognized
>
> I think the error should look as:
> a2=# CREATE SEQUENCE s SEQUENCE__ NAME s;
> ERROR: syntax error at or near "SEQUENCE__"
> LINE 1: CREATE SEQUENCE s SEQUENCE__ NAME s;
> ^
Fixed by improving message.
> 13. Also strange error message:
> test=# CREATE SCHEMA sch;
> test=# ALTER TABLE idnt ALTER COLUMN n1 ADD GENERATED BY DEFAULT AS
> IDENTITY (SEQUENCE NAME sch.s START 1);
> ERROR: relation "sch.idnt" does not exist
>
> But if a table sch.idnt exists, it leads to a silent inconsistency:
> test=# CREATE TABLE sch.idnt(n1 int);
> CREATE TABLE
> test=# ALTER TABLE idnt ALTER COLUMN n1 ADD GENERATED BY DEFAULT AS
> IDENTITY (SEQUENCE NAME sch.s START 1);
> ALTER TABLE
> test=# DROP TABLE sch.idnt;
> ERROR: could not find tuple for attrdef 0
I can't reproduce this. Can you give me a complete command sequence
from scratch?
> 14. Wrong hint message:
> test=# DROP SEQUENCE idnt_i_seq;
> ERROR: cannot drop sequence idnt_i_seq because default for table idnt
> column i requires it
> HINT: You can drop default for table idnt column i instead.
>
> but according to DDL there is no "default" which can be dropped:
> test=# ALTER TABLE idnt ALTER COLUMN i DROP DEFAULT;
> ERROR: column "i" of relation "idnt" is an identity column
fixed (added errhint)
> 15. And finally. The command:
> test=# CREATE TABLE t(i int GENERATED BY DEFAULT AS IDENTITY (SEQUENCE NAME s));
>
> leads to a core dump.
> It happens when no sequence parameter (like "START") is set.
fixed
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Vitaly, will you be able to review this again?
On 2/28/17 21:23, Peter Eisentraut wrote:
> New patch that fixes everything. ;-) Besides hopefully addressing all
> your issues, this version also no longer requires separate privileges on
> the internal sequence, which was the last outstanding functional issue
> that I am aware of. This version also no longer stores a default entry
> in pg_attrdef. Instead, the rewriter makes up the default expression on
> the fly. This makes some things much simpler.
>
> On 1/4/17 19:34, Vitaly Burovoy wrote:
>> 1. The fact COPY ignores GENERATED ALWAYS constraint (treats as
>> GENERATED BY DEFAULT) should be mentioned as well as rules.
>
> fixed (documentation added)
>
> What do you mean for rules?
>
>> 2. Usually error message for identical columns (with LIKE clause)
>> looks like this:
>> test=# CREATE TABLE def(i serial, n1 int NOT NULL, n2 int);
>> CREATE TABLE
>> test=# CREATE TABLE test(i serial, LIKE def INCLUDING ALL);
>> ERROR: column "i" specified more than once
>>
>> but for generated columns it is disorienting enough:
>> test=# CREATE TABLE idnt(i int GENERATED BY DEFAULT AS IDENTITY);
>> CREATE TABLE
>> test=# CREATE TABLE test(i int GENERATED BY DEFAULT AS IDENTITY, LIKE
>> idnt INCLUDING ALL);
>> ERROR: relation "test_i_seq" already exists
>
> Yeah, this is kind of hard to fix though, because the sequence names
> are decided during parse analysis, when we don't see that the same
> sequence name is also used by another new column. We could maybe
> patch around it in an ugly way, but it doesn't seem worth it for this
> marginal case.
>
>> 3. Strange error (not about absence of a column; but see pp.5 and 8):
>> test=# ALTER TABLE idnt ALTER COLUMN o ADD GENERATED ALWAYS AS IDENTITY;
>> ERROR: identity column type must be smallint, integer, or bigint
>
> What's wrong with that?
>
>> 4. Altering type leads to an error:
>> test=# ALTER TABLE idnt ALTER COLUMN i TYPE bigint;
>> ERROR: cannot alter data type of identity column "i"
>>
>> it is understandable for non-integers. But why do you forbid changing
>> to the supported types?
>
> fixed (is now allowed)
>
>> 5. Attempt to make a column be identity fails:
>> test=# ALTER TABLE idnt ALTER COLUMN n1 SET GENERATED BY DEFAULT;
>> ERROR: column "n1" of relation "idnt" is not an identity column
>>
>> As I understand from the Spec, chapter 11.20 General Rule 2)b) says
>> about the final state of a column without mentioning of the initial
>> state.
>> Therefore even if the column has the initial state "not generated",
>> after the command it changes the state to either "GENERATED ALWAYS" or
>> "GENERATED BY DEFAULT".
>
> 11.12 <alter column definition> states that "If <alter identity column
> specification> or <drop identity property clause> is specified, then C
> shall be an identity column." So this clause is only to alter an
> existing identity column, not make a new one.
>
>> 6. The docs mention a syntax:
>> ALTER [ COLUMN ] <replaceable
>> class="PARAMETER">column_name</replaceable> { SET GENERATED { ALWAYS |
>> BY DEFAULT } | SET <replaceable>sequence_option</replaceable> | RESET
>> } [...]
>>
>> but the command fails:
>> test=# ALTER TABLE idnt ALTER COLUMN i RESET;
>> ERROR: syntax error at or near ";"
>> LINE 1: ALTER TABLE idnt ALTER COLUMN i RESET;
>
> This works for me. Check again please.
>
>> 7. (the same line of the doc)
>> usually ellipsis appears in inside parenthesis with clauses can be
>> repeated, in other words it should be written as:
>> ALTER <skipped> column_name</replaceable> ( { SET GENERATED { ALWAYS |
>> BY DEFAULT } | <skipped> } [...] )
>
> But there are no parentheses in that syntax. I think the syntax
> synopsis as written is correct.
>
>> 8. To avoid unnecessary syntax "ALTER COLUMN ... ADD GENERATED ..."
>> and use existing syntax "ALTER COLUMN ... SET GENERATED ..." you can
>> just make the "SET" word be optional as a PG's extension. I.e. instead
>> of:
>> test=# ALTER TABLE idnt ALTER i SET GENERATED ALWAYS SET INCREMENT BY 4;
>>
>> you can write:
>> test=# ALTER TABLE idnt ALTER i SET GENERATED ALWAYS INCREMENT BY 4;
>> test=# ALTER TABLE idnt ALTER n1 SET GENERATED ALWAYS INCREMENT BY 4;
>> -- which sets identity constraint - see p.5
>>
>> which is very similar to your extended syntax:
>> test=# ALTER TABLE idnt ALTER n1 ADD GENERATED ALWAYS AS IDENTITY
>> (INCREMENT BY 4);
>
> See under 5 that that is not correct.
>
>> 9. The command fails:
>> test=# ALTER TABLE idnt ALTER n2 ADD GENERATED ALWAYS AS IDENTITY;
>> ERROR: column "n2" of relation "idnt" must be declared NOT NULL
>> before identity can be added
>>
>> whereas the Spec does not contains a requirement for a column to be a
>> NOT NULLABLE.
>> You can implicitly set a column as NOT NULL (as the "serial" macros
>> does), but not require it later.
>
> The spec requires that an identity column is NOT NULL.
>
>> Moreover you can get a NULLABLE identity column by:
>> test=# ALTER TABLE idnt ALTER COLUMN i DROP NOT NULL;
>> ALTER TABLE
>> test=# \d idnt
>> Table "public.idnt"
>> Column | Type | Collation | Nullable | Default
>> --------+---------+-----------+----------+------------------------------
>> i | integer | | | generated always as identity
>> n1 | integer | | not null |
>> n2 | integer | | |
>
> Fixed by disallowing that command (similar to dropping NOT NULL from a
> primary key, for example).
>
>> 10. Inherited tables are not allowed at all
>
> fixed
>
>
>> 11. The last CF added partitioned tables which are similar to
>> inherited, but slightly different.
>
> fixed
>
>> 12. You've introduced a new parameter for a sequence declaration
>> ("SEQUENCE NAME") which is not mentioned in the docs and not
>> supported:
>> a2=# CREATE SEQUENCE s SEQUENCE NAME s;
>> ERROR: option "sequence_name" not recognized
>>
>> I think the error should look as:
>> a2=# CREATE SEQUENCE s SEQUENCE__ NAME s;
>> ERROR: syntax error at or near "SEQUENCE__"
>> LINE 1: CREATE SEQUENCE s SEQUENCE__ NAME s;
>> ^
>
> Fixed by improving message.
>
>> 13. Also strange error message:
>> test=# CREATE SCHEMA sch;
>> test=# ALTER TABLE idnt ALTER COLUMN n1 ADD GENERATED BY DEFAULT AS
>> IDENTITY (SEQUENCE NAME sch.s START 1);
>> ERROR: relation "sch.idnt" does not exist
>>
>> But if a table sch.idnt exists, it leads to a silent inconsistency:
>> test=# CREATE TABLE sch.idnt(n1 int);
>> CREATE TABLE
>> test=# ALTER TABLE idnt ALTER COLUMN n1 ADD GENERATED BY DEFAULT AS
>> IDENTITY (SEQUENCE NAME sch.s START 1);
>> ALTER TABLE
>> test=# DROP TABLE sch.idnt;
>> ERROR: could not find tuple for attrdef 0
>
> I can't reproduce this. Can you give me a complete command sequence
> from scratch?
>
>> 14. Wrong hint message:
>> test=# DROP SEQUENCE idnt_i_seq;
>> ERROR: cannot drop sequence idnt_i_seq because default for table idnt
>> column i requires it
>> HINT: You can drop default for table idnt column i instead.
>>
>> but according to DDL there is no "default" which can be dropped:
>> test=# ALTER TABLE idnt ALTER COLUMN i DROP DEFAULT;
>> ERROR: column "i" of relation "idnt" is an identity column
>
> fixed (added errhint)
>
>> 15. And finally. The command:
>> test=# CREATE TABLE t(i int GENERATED BY DEFAULT AS IDENTITY (SEQUENCE NAME s));
>>
>> leads to a core dump.
>> It happens when no sequence parameter (like "START") is set.
>
> fixed
>
>
>
>
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/15/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > Vitaly, will you be able to review this again? > > -- > Peter Eisentraut http://www.2ndQuadrant.com/ I apologize for a delay. Yes, I'm going to do it by Sunday. -- Best regards, Vitaly Burovoy
On 2/28/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> New patch that fixes everything. ;-)
Great work!
> On 1/4/17 19:34, Vitaly Burovoy wrote:
>> 1. The fact COPY ignores GENERATED ALWAYS constraint (treats as
>> GENERATED BY DEFAULT) should be mentioned as well as rules.
>
> fixed (documentation added)
>
> What do you mean for rules?
I meant the phrase "However, it will not invoke rules." mentioned there.
For rewrite rules this behavior is mentioned on the COPY page, not on
the "CREATE RULE" page.
I think it would be better if that behavior is placed on both CREATE
TABLE and COPY pages.
>> 2. Usually error message for identical columns (with LIKE clause)
>> looks like this:
>> test=# CREATE TABLE def(i serial, n1 int NOT NULL, n2 int);
>> CREATE TABLE
>> test=# CREATE TABLE test(i serial, LIKE def INCLUDING ALL);
>> ERROR: column "i" specified more than once
>>
>> but for generated columns it is disorienting enough:
>> test=# CREATE TABLE idnt(i int GENERATED BY DEFAULT AS IDENTITY);
>> CREATE TABLE
>> test=# CREATE TABLE test(i int GENERATED BY DEFAULT AS IDENTITY, LIKE
>> idnt INCLUDING ALL);
>> ERROR: relation "test_i_seq" already exists
>
> Yeah, this is kind of hard to fix though, because the sequence names
> are decided during parse analysis, when we don't see that the same
> sequence name is also used by another new column. We could maybe
> patch around it in an ugly way, but it doesn't seem worth it for this
> marginal case.
OK
>> 3. Strange error (not about absence of a column; but see pp.5 and 8):
>> test=# ALTER TABLE idnt ALTER COLUMN o ADD GENERATED ALWAYS AS IDENTITY;
>> ERROR: identity column type must be smallint, integer, or bigint
>
> What's wrong with that?
The column mentioned there does not exist. Therefore the error should
be about it, not about a type of absent column:
test=# CREATE TABLE idnt(i int GENERATED BY DEFAULT AS IDENTITY, t TEXT);
CREATE TABLE
test=# ALTER TABLE idnt ALTER COLUMN o TYPE int; -- expected error message
ERROR: 42703: column "o" of relation "idnt" does not exist
test=# -- compare with:
test=# ALTER TABLE idnt ALTER COLUMN o ADD GENERATED ALWAYS AS
IDENTITY; -- strange error message
ERROR: identity column type must be smallint, integer, or bigint
>> 5. Attempt to make a column be identity fails:
>> test=# ALTER TABLE idnt ALTER COLUMN n1 SET GENERATED BY DEFAULT;
>> ERROR: column "n1" of relation "idnt" is not an identity column
>>
>> As I understand from the Spec, chapter 11.20 General Rule 2)b) says
>> about the final state of a column without mentioning of the initial
>> state.
>> Therefore even if the column has the initial state "not generated",
>> after the command it changes the state to either "GENERATED ALWAYS" or
>> "GENERATED BY DEFAULT".
>
> 11.12 <alter column definition> states that "If <alter identity column
> specification> or <drop identity property clause> is specified, then C
> shall be an identity column." So this clause is only to alter an
> existing identity column, not make a new one.
Ouch. Right. The rule was at the upper level.
Nevertheless even now we don't follow rules directly.
For example, we allow "SET NOT NULL" and "TYPE" for the column which
are restricted by the spec.
Since we extend the spec by "ADD GENERATED...", "SEQUENCE NAME" and
allow more than one identity column, why can't we extend it by
allowing "SET GENERATED" for non-identity columns and drop "ADD
GENERATED..."?
>> 6. The docs mention a syntax:
>> ALTER [ COLUMN ] <replaceable
>> class="PARAMETER">column_name</replaceable> { SET GENERATED { ALWAYS |
>> BY DEFAULT } | SET <replaceable>sequence_option</replaceable> | RESET
>> } [...]
>>
>> but the command fails:
>> test=# ALTER TABLE idnt ALTER COLUMN i RESET;
>> ERROR: syntax error at or near ";"
>> LINE 1: ALTER TABLE idnt ALTER COLUMN i RESET;
>
> This works for me. Check again please.
I'm sorry, it still does not work for me (whether you mix it up with "RESTART"):
test=# ALTER TABLE idnt ALTER COLUMN i RESET;
ERROR: syntax error at or near ";"
LINE 1: ALTER TABLE idnt ALTER COLUMN i RESET;
>> 7. (the same line of the doc)
>> usually ellipsis appears in inside parenthesis with clauses can be
>> repeated, in other words it should be written as:
>> ALTER <skipped> column_name</replaceable> ( { SET GENERATED { ALWAYS |
>> BY DEFAULT } | <skipped> } [...] )
>
> But there are no parentheses in that syntax. I think the syntax
> synopsis as written is correct.
I was wrong. Your version is correct.
>> 9. The command fails:
>> test=# ALTER TABLE idnt ALTER n2 ADD GENERATED ALWAYS AS IDENTITY;
>> ERROR: column "n2" of relation "idnt" must be declared NOT NULL
>> before identity can be added
>>
>> whereas the Spec does not contains a requirement for a column to be a
>> NOT NULLABLE.
>> You can implicitly set a column as NOT NULL (as the "serial" macros
>> does), but not require it later.
>
> The spec requires that an identity column is NOT NULL.
OK. "11.4 SR 16)d" really says "The <column constraint definition> NOT
NULL NOT DEFERRABLE is implicit."
>> 11. The last CF added partitioned tables which are similar to
>> inherited, but slightly different.
>
> fixed
Something is left to be fixed:
test=# CREATE TABLE measurement (
test(# i int GENERATED BY DEFAULT AS IDENTITY,
test(# logdate date not null,
test(# peaktemp int,
test(# unitsales int
test(# ) PARTITION BY RANGE (logdate);
CREATE TABLE
test=# CREATE TABLE measurement_y2016m07
test-# PARTITION OF measurement (
test(# unitsales DEFAULT 0
test(# ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
CREATE TABLE
test=# ALTER TABLE MEASUREMENT ALTER i RESTART 2;
ERROR: column "i" of relation "measurement_y2016m07" is not an identity column
>> 13. Also strange error message:
>> ...
>> test=# DROP TABLE sch.idnt;
>> ERROR: could not find tuple for attrdef 0
>
> I can't reproduce this. Can you give me a complete command sequence
> from scratch?
Since you don't use defaults (which are linked by the "attrdef") it is
not relevant now.
> --
> Peter Eisentraut http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
The patch should be rebased to the current master. It has easy
conflicts in describe.c in the commit
395bfaae8e786eb17fc9dd79e4636f97c9d9b820.
Please, don't include the file catversion.h in it because it is
changed often and leads to error when applying.
New changes fix old bugs and introduce new ones.
16. changing a type does not change an underlying sequence type (its limits):
test=# CREATE TABLE itest3 (a smallint generated by default as
identity (start with 32767 increment by 5), b text);
CREATE TABLE
test=# ALTER TABLE itest3 ALTER a TYPE bigint;
ALTER TABLE
test=# INSERT INTO itest3(b) VALUES ('a');
INSERT 0 1
test=# INSERT INTO itest3(b) VALUES ('b');
ERROR: nextval: reached maximum value of sequence "itest3_a_seq" (32767)
On the one hand according to the spec it is not possible to change a
type of an identity column, on the other hand the spec says nothing
about different numeric types (int2, int4, int8). The worst thing is
that it is hard to understand which sequence is used (without a
"default") and since the ALTER TABLE is finished without errors users
may think everything is OK, but really it is not.
17. how to restart a sequence?
test=# ALTER TABLE itest3 ALTER a SET RESTART 2;
ERROR: sequence option "restart" not supported here
LINE 1: ALTER TABLE itest3 ALTER a SET RESTART 2;
Khm. The "RESTART" is one of official "sequence_options" (comparable
to the "SEQUENCE NAME"), why it is not allowed?
But there is another DDL which works OK, but not reflected in the docs:
test=# ALTER TABLE itest3 ALTER a RESTART 2;
ALTER TABLE
18. If there is no sequence owned by a column, all DDLs for a sequence
behind an identity column do not raise errors but do nothing:
test=# CREATE TABLE itest3 (a int generated by default as identity
(start with 7), b text);
CREATE TABLE
test=# ALTER SEQUENCE itest3_a_seq OWNED BY NONE;
ALTER SEQUENCE
test=# ALTER TABLE itest3 ALTER a SET START 10;
ALTER TABLE
test=# -- sequence has not changed
test=# \d itest3_a_seq
Sequence "public.itest3_a_seq" Column | Type | Value
------------+---------+-------last_value | bigint | 7log_cnt | bigint | 0is_called | boolean | f
test=# -- the table is still "generated by default"
test=# \d itest3 Table "public.itest3"Column | Type | Collation | Nullable |
Default
--------+---------+-----------+----------+----------------------------------a | integer | | not null |
generatedby default as identityb | text | | |
test=# INSERT INTO itest3(b)VALUES('a'); -- errors appear only when it
is changing
ERROR: no owned sequence found
and the table will be dumped without errors with columns as non-identity.
19. If anyone occasionally drops OWNED BY for the linked sequence
there is no ways to restore it "as was":
test=# CREATE TABLE itest3 (a int generated by default as identity, b text);
CREATE TABLE
test=# ALTER SEQUENCE itest3_a_seq OWNED BY NONE; -- erroneous
ALTER SEQUENCE
test=# INSERT INTO itest3(b)VALUES('a'); -- Ouch! Error!!!
ERROR: no owned sequence found
test=# ALTER SEQUENCE itest3_a_seq OWNED BY itest3.a; -- try to restore
ALTER SEQUENCE
test=# INSERT INTO itest3(b)VALUES('a');
INSERT 0 1
It seems it is OK, all works perfect. But after dump/restore the
column looses the "identity" property:
test2=# \d itest3 Table "public.itest3"Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------a | integer | | not null |b | text |
| |
It is a hidden bomb, because process of restoring seems normal (which
means dumps are OK), but a users' code will not work correctly.
20. sequence does not follow the table owned by:
test=# CREATE TABLE itest3 (a int generated by default as identity, b text);
CREATE TABLE
test=# CREATE SCHEMA sch;
CREATE SCHEMA
test=# ALTER TABLE itest3 SET SCHEMA sch;
ALTER TABLE
test=# \d List of relationsSchema | Name | Type | Owner
--------+--------------+----------+----------public | itest3_a_seq | sequence | postgres
(
(1 row)
test=# \d sch.* Table "sch.itest3"Column | Type | Collation | Nullable |
Default
--------+---------+-----------+----------+----------------------------------a | integer | | not null |
generatedby default as identityb | text | | |
Also dump/restore fails with:
ERROR: relation "itest3" does not exist
21. There are many places where error codes look strange:
test=# ALTER TABLE itest3 ALTER a SET SEQUENCE NAME "ww"; -- expected
error code
ERROR: 42601: invalid sequence option SEQUENCE NAME
LINE 1: ALTER TABLE itest3 ALTER a SET SEQUENCE NAME "ww";
42601 = syntax_error
but
test=# ALTER TABLE itest3 ALTER b SET GENERATED BY DEFAULT; --
whether it is "internal_error" or user's error?
ERROR: XX000: column "b" of relation "itest3" is not an identity column
XX000 = internal_error
whether the error 42611 (invalid_column_definition) is good enough for it?
--
Best regards,
Vitaly Burovoy
On 3/20/17 05:43, Vitaly Burovoy wrote:
>>> 1. The fact COPY ignores GENERATED ALWAYS constraint (treats as
>>> GENERATED BY DEFAULT) should be mentioned as well as rules.
> I think it would be better if that behavior is placed on both CREATE
> TABLE and COPY pages.
done
>>> 3. Strange error (not about absence of a column; but see pp.5 and 8):
>>> test=# ALTER TABLE idnt ALTER COLUMN o ADD GENERATED ALWAYS AS IDENTITY;
>>> ERROR: identity column type must be smallint, integer, or bigint
>>
>> What's wrong with that?
>
> The column mentioned there does not exist. Therefore the error should
> be about it, not about a type of absent column:
This was already fixed in the previous version.
> Since we extend the spec by "ADD GENERATED...", "SEQUENCE NAME" and
> allow more than one identity column, why can't we extend it by
> allowing "SET GENERATED" for non-identity columns and drop "ADD
> GENERATED..."?
SQL commands generally don't work that way. Either they create or they
alter. There are "OR REPLACE" options when you do both. So I think it
is better to keep these two things separate. Also, while you argue that
we *could* do it the way you propose, I don't really see an argument why
it would be better.
>>> 6. The docs mention a syntax:
>>> ALTER [ COLUMN ] <replaceable
>>> class="PARAMETER">column_name</replaceable> { SET GENERATED { ALWAYS |
>>> BY DEFAULT } | SET <replaceable>sequence_option</replaceable> | RESET
>>> } [...]
>>>
>>> but the command fails:
>>> test=# ALTER TABLE idnt ALTER COLUMN i RESET;
>>> ERROR: syntax error at or near ";"
>>> LINE 1: ALTER TABLE idnt ALTER COLUMN i RESET;
>>
>> This works for me. Check again please.
>
> I'm sorry, it still does not work for me (whether you mix it up with "RESTART"):
> test=# ALTER TABLE idnt ALTER COLUMN i RESET;
> ERROR: syntax error at or near ";"
> LINE 1: ALTER TABLE idnt ALTER COLUMN i RESET;
Oh I see, the documentation was wrong. The correct syntax is RESTART
rather than RESET. Fixed the documentation.
>>> 11. The last CF added partitioned tables which are similar to
>>> inherited, but slightly different.
>>
>> fixed
>
> Something is left to be fixed:
> test=# CREATE TABLE measurement (
> test(# i int GENERATED BY DEFAULT AS IDENTITY,
> test(# logdate date not null,
> test(# peaktemp int,
> test(# unitsales int
> test(# ) PARTITION BY RANGE (logdate);
> CREATE TABLE
> test=# CREATE TABLE measurement_y2016m07
> test-# PARTITION OF measurement (
> test(# unitsales DEFAULT 0
> test(# ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
> CREATE TABLE
> test=# ALTER TABLE MEASUREMENT ALTER i RESTART 2;
> ERROR: column "i" of relation "measurement_y2016m07" is not an identity column
fixed
> The patch should be rebased to the current master. It has easy
> conflicts in describe.c in the commit
> 395bfaae8e786eb17fc9dd79e4636f97c9d9b820.
done
> Please, don't include the file catversion.h in it because it is
> changed often and leads to error when applying.
OK
> 16. changing a type does not change an underlying sequence type (its limits):
It does change the type, but changing the type doesn't change the
limits. That is a property of how ALTER SEQUENCE works, which was
separately discussed.
> 17. how to restart a sequence?
> test=# ALTER TABLE itest3 ALTER a SET RESTART 2;
> ERROR: sequence option "restart" not supported here
> LINE 1: ALTER TABLE itest3 ALTER a SET RESTART 2;
>
> Khm. The "RESTART" is one of official "sequence_options" (comparable
> to the "SEQUENCE NAME"), why it is not allowed?
> But there is another DDL which works OK, but not reflected in the docs:
> test=# ALTER TABLE itest3 ALTER a RESTART 2;
> ALTER TABLE
Yes, that is now fixed. See #6 above.
> 18. If there is no sequence owned by a column, all DDLs for a sequence
> behind an identity column do not raise errors but do nothing:
> test=# CREATE TABLE itest3 (a int generated by default as identity
> (start with 7), b text);
> CREATE TABLE
> test=# ALTER SEQUENCE itest3_a_seq OWNED BY NONE;
> ALTER SEQUENCE
I have changed it to prohibit changing OWNED BY of an identity sequence
directly, so this can't happen anymore.
> 19. If anyone occasionally drops OWNED BY for the linked sequence
> there is no ways to restore it "as was":
fixed, see above
> 20. sequence does not follow the table owned by:
fixed
> 21. There are many places where error codes look strange:
> test=# ALTER TABLE itest3 ALTER b SET GENERATED BY DEFAULT; --
> whether it is "internal_error" or user's error?
> ERROR: XX000: column "b" of relation "itest3" is not an identity column
I added error codes to the places that were missing them.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
I haven't seen a patch (I'll do it later), just few notes: On 3/21/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: >>>> 3. Strange error (not about absence of a column; but see pp.5 and 8): >>>> test=# ALTER TABLE idnt ALTER COLUMN o ADD GENERATED ALWAYS AS >>>> IDENTITY; >>>> ERROR: identity column type must be smallint, integer, or bigint >>> >>> What's wrong with that? >> >> The column mentioned there does not exist. Therefore the error should >> be about it, not about a type of absent column: > > This was already fixed in the previous version. I've just checked and still get an error about a type, not about absence of a column: test=# CREATE TABLE itest3 (a int generated by default as identity, b text); CREATE TABLE test=# ALTER TABLE itest3 ALTER COLUMN o ADD GENERATED ALWAYS AS IDENTITY; ERROR: identity column type must be smallint, integer, or bigint >> Since we extend the spec by "ADD GENERATED...", "SEQUENCE NAME" and >> allow more than one identity column, why can't we extend it by >> allowing "SET GENERATED" for non-identity columns and drop "ADD >> GENERATED..."? > > SQL commands generally don't work that way. Either they create or they > alter. There are "OR REPLACE" options when you do both. I'd agree with you if we are talking about database's objects like tables, functions, columns etc. > So I think it > is better to keep these two things separate. Also, while you argue that > we *could* do it the way you propose, I don't really see an argument why > it would be better. My argument is consistency. Since IDENTITY is a property of a column (similar to DEFAULT, NOT NULL, attributes, STORAGE, etc.), it follows a different rule: it is either set or not set. If it did not set before, the "SET" DDL "adds" it, if that property already present, the DDL replaces it. There is no "ADD" clause in DDLs like "...ALTER table ALTER column..." (only "SET", "RESET" and "DROP")[2]. Your patch introduces the single DDL version with "...ALTER column ADD..." for a property. >> 16. changing a type does not change an underlying sequence type (its >> limits): > > It does change the type, but changing the type doesn't change the > limits. That is a property of how ALTER SEQUENCE works, which was > separately discussed. Are you about the thread[1]? If so, I'd say the current behavior is not good. I sent an example with users' bad experience who will know nothing about sequences (because they'll deal with identity columns). Would it be better to change bounds of a sequence if they match the bounds of an old type (to the bounds of a new type)? > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services [1] https://www.postgresql.org/message-id/flat/898deb94-5265-37cf-f199-4f79ef864536@2ndquadrant.com [2] https://www.postgresql.org/docs/current/static/sql-altertable.html -- Best regards, Vitaly Burovoy
On 3/21/17 16:11, Vitaly Burovoy wrote: > I've just checked and still get an error about a type, not about > absence of a column: > test=# CREATE TABLE itest3 (a int generated by default as identity, b text); > CREATE TABLE > test=# ALTER TABLE itest3 ALTER COLUMN o ADD GENERATED ALWAYS AS IDENTITY; > ERROR: identity column type must be smallint, integer, or bigint OK, I have a fix. > My argument is consistency. > Since IDENTITY is a property of a column (similar to DEFAULT, NOT > NULL, attributes, STORAGE, etc.), it follows a different rule: it is > either set or not set. If it did not set before, the "SET" DDL "adds" > it, if that property already present, the DDL replaces it. > There is no "ADD" clause in DDLs like "...ALTER table ALTER column..." > (only "SET", "RESET" and "DROP")[2]. > Your patch introduces the single DDL version with "...ALTER column > ADD..." for a property. But it creates a sequence, so it creates state. So mistakes could easily be masked. With my patch, if you do ADD twice, you get an error.With your proposal, you'd have to use SET, and youcould overwrite existing sequence state without realizing it. >> It does change the type, but changing the type doesn't change the >> limits. That is a property of how ALTER SEQUENCE works, which was >> separately discussed. > > Are you about the thread[1]? If so, I'd say the current behavior is not good. > I sent an example with users' bad experience who will know nothing > about sequences (because they'll deal with identity columns). > Would it be better to change bounds of a sequence if they match the > bounds of an old type (to the bounds of a new type)? That's an idea, but that's for a separate patch. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/21/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 3/21/17 16:11, Vitaly Burovoy wrote: >> My argument is consistency. >> Since IDENTITY is a property of a column (similar to DEFAULT, NOT >> NULL, attributes, STORAGE, etc.), it follows a different rule: it is >> either set or not set. If it did not set before, the "SET" DDL "adds" >> it, if that property already present, the DDL replaces it. >> There is no "ADD" clause in DDLs like "...ALTER table ALTER column..." >> (only "SET", "RESET" and "DROP")[2]. >> Your patch introduces the single DDL version with "...ALTER column >> ADD..." for a property. > > But it creates a sequence, so it creates state. Right. But it is an internal mechanism. DDL is not about creating a sequence, it is about changing a property. > So mistakes could easily be masked. With my patch, if you do ADD twice, you get an error. Agree. But what's for? Whether that parameters are incompatible (and can't be changed later)? > With your proposal, you'd have to use SET, and you could overwrite > existing sequence state without realizing it. I can't overwrite its state (current value), only its settings like start, maxval, etc. In fact when I write a DDL I want to change a schema. For non-properties it is natural to write "CREATE" (schema, table) and "ADD" (column, constraints) because there can be many of them (with different names) in a single object: many schemas in a DB, many tables in a schema, many columns in a table and even many constraints in a table. So ADD is used for adding objects which have a name to some container (DB, schema, table). It is not true for the IDENTITY property. You can have many identity columns, but you can not have many of them in a single column. Column's IDENTITY behavior is very similar to a DEFAULT one. We write "SET DEFAULT" and don't care whether it was set before or not, because we can't have many of them for a single column. Why should we do that for IDENTITY? Whether I write "ADD" or "SET" I want to have a column with some behavior and I don't mind what behavior it has until it is incompatible with my wish (e.g. it has DEFAULT, but I want IDENTITY or vice versa). >>> It does change the type, but changing the type doesn't change the >>> limits. That is a property of how ALTER SEQUENCE works, which was >>> separately discussed. >> >> Are you about the thread[1]? If so, I'd say the current behavior is not >> good. >> I sent an example with users' bad experience who will know nothing >> about sequences (because they'll deal with identity columns). >> Would it be better to change bounds of a sequence if they match the >> bounds of an old type (to the bounds of a new type)? > > That's an idea, but that's for a separate patch. It is very likely to have one in Postgres10. I'm afraid in the other case we'll impact with many bug reports similar to my example. > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services > -- Best regards, Vitaly Burovoy
On 3/22/17 03:59, Vitaly Burovoy wrote: > Column's IDENTITY behavior is very similar to a DEFAULT one. We write > "SET DEFAULT" and don't care whether it was set before or not, because > we can't have many of them for a single column. Why should we do that > for IDENTITY? One indication is that the SQL standard requires that DROP IDENTITY only succeed if the column is currently an identity column. That is different from how DEFAULT works. Another difference is that there is no such thing as "no default", because in absence of an explicit default, it is NULL. So all you are doing with SET DEFAULT or DROP DEFAULT is changing the default. You are not actually adding or removing it. Therefore, the final effect of SET DEFAULT is the same no matter whether another default was there before or not. For ADD/SET IDENTITY, you get different behaviors. For example: ADD .. AS IDENTITY (START 2) creates a new sequence that starts at 2 and uses default parameters otherwise. But SET (START 2) alters the start parameter of an existing sequence. So depending on whether you already have an identity sequence, these commands do completely different things. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/22/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 3/22/17 03:59, Vitaly Burovoy wrote: >> Column's IDENTITY behavior is very similar to a DEFAULT one. We write >> "SET DEFAULT" and don't care whether it was set before or not, because >> we can't have many of them for a single column. Why should we do that >> for IDENTITY? > > One indication is that the SQL standard requires that DROP IDENTITY only > succeed if the column is currently an identity column. That is > different from how DEFAULT works. I think we'll end up with "DROP IDENTITY IF EXISTS" to avoid raising an exception and "ADD OR SET" if your grammar remains. > Another difference is that there is no such thing as "no default", > because in absence of an explicit default, it is NULL. So all you are > doing with SET DEFAULT or DROP DEFAULT is changing the default. You are > not actually adding or removing it. Right. From that PoV IDENTITY also changes a default value: "SET (ADD ... AS?) IDENTITY" works as setting a default to "nextval(...)" whereas "DROP IDENTITY" works as setting it back to NULL. > Therefore, the final effect of SET DEFAULT is the same no matter whether > another default was there before or not. For ADD/SET IDENTITY, you get > different behaviors. For example: > > ADD .. AS IDENTITY (START 2) > > creates a new sequence that starts at 2 and uses default parameters > otherwise. But > > SET (START 2) > > alters the start parameter of an existing sequence. So depending on > whether you already have an identity sequence, these commands do > completely different things. If you use "SET START 2" to a non-identity columns, you should get an exception (no information about an identity type: neither "by default" nor "always"). The correct example is: ADD GENERATED BY DEFAULT AS IDENTITY (START 2) and SET GENERATED BY DEFAULT SET START 2 Note that creating a sequence is an internal machinery hidden from users. Try to see from user's PoV: the goal is to have a column with an autoincrement. If it is already autoincremented, no reason to create a sequence (it is already present) and no reason to restart with 2 (there can be rows with such numbers). "... SET START 2" is for the next "RESTART" DDL, and if a user insists to start with 2, it is still possible: SET GENERATED BY DEFAULT SET START 2 RESTART 2 I still think that introducing "ADD" for a property which can not be used more than once (compare with "ADD CHECK": you can use it with the same expression multiple times) is not a good idea. I think there should be a consensus in the community for a grammar. > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Best regards, Vitaly Burovoy
On 3/23/17 06:09, Vitaly Burovoy wrote: > I think we'll end up with "DROP IDENTITY IF EXISTS" to avoid raising > an exception and "ADD OR SET" if your grammar remains. That sounds reasonable to me. > Right. From that PoV IDENTITY also changes a default value: "SET (ADD > ... AS?) IDENTITY" works as setting a default to "nextval(...)" > whereas "DROP IDENTITY" works as setting it back to NULL. But dropping and re-adding an identity destroys state, so it's not quite the same. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/23/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 3/23/17 06:09, Vitaly Burovoy wrote: >> I think we'll end up with "DROP IDENTITY IF EXISTS" to avoid raising >> an exception and "ADD OR SET" if your grammar remains. > > That sounds reasonable to me. It would be great if "DROP IDENTITY IF EXISTS" is in the current patch since we don't have any disagreements about "DROP IDENTITY" behavior and easiness of implementation of "IF EXISTS" there. >> Right. From that PoV IDENTITY also changes a default value: "SET (ADD >> ... AS?) IDENTITY" works as setting a default to "nextval(...)" >> whereas "DROP IDENTITY" works as setting it back to NULL. > > But dropping and re-adding an identity destroys state, so it's not quite > the same. How does dropping and re-adding affects a choosing between "ADD" and "SET"? If you drop identity property, you get a column's state destroyed whatever grammar variation you are using. > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services I have an idea. What about the next version of DDL: DROP IDENTITY [ IF EXISTS ] SET GENERATED { ALWAYS | BY DEFAULT } [ IF NOT EXISTS ] | SET ... That version: 1. does not introduce a new "ADD" variation; 2. without "IF NOT EXISTS" follows the standard; 3. with "IF NOT EXISTS" sets a column's identity property or alters it (works as "CREATE OR REPLACE" for functions). -- Best regards, Vitaly Burovoy
On 3/24/17 05:29, Vitaly Burovoy wrote: > It would be great if "DROP IDENTITY IF EXISTS" is in the current patch > since we don't have any disagreements about "DROP IDENTITY" behavior > and easiness of implementation of "IF EXISTS" there. Here is an updated patch that adds DROP IDENTITY IF EXISTS. Unfortunately, implementing ADD IF NOT EXISTS is much harder, so I haven't done that here. Additionally, this patch fixes a few minor issues that you had pointed out, and merges with the new expression evaluation system in the executor. I have also CC'ed you on a separate patch to improve the behavior when changing a sequence's data type. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/29/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 3/24/17 05:29, Vitaly Burovoy wrote: >> It would be great if "DROP IDENTITY IF EXISTS" is in the current patch >> since we don't have any disagreements about "DROP IDENTITY" behavior >> and easiness of implementation of "IF EXISTS" there. > > Here is an updated patch that adds DROP IDENTITY IF EXISTS. > > Unfortunately, implementing ADD IF NOT EXISTS is much harder, so I > haven't done that here. > > Additionally, this patch fixes a few minor issues that you had pointed > out, and merges with the new expression evaluation system in the executor. > > I have also CC'ed you on a separate patch to improve the behavior when > changing a sequence's data type. Thank you a lot. I'll have a deep look by the Sunday evening. Why do you still want to leave "ADD IF NOT EXISTS" instead of using "SET IF NOT EXISTS"? If someone wants to follow the standard he can simply not to use "IF NOT EXISTS" at all. Without it error should be raised. But we would not need to introduce a new grammar for a column property which is single and can't be added more than once. -- Best regards, Vitaly Burovoy
On 2017-03-29 13:40:29 -0400, Peter Eisentraut wrote:
> On 3/24/17 05:29, Vitaly Burovoy wrote:
> > It would be great if "DROP IDENTITY IF EXISTS" is in the current patch
> > since we don't have any disagreements about "DROP IDENTITY" behavior
> > and easiness of implementation of "IF EXISTS" there.
>
> Here is an updated patch that adds DROP IDENTITY IF EXISTS.
>
> Unfortunately, implementing ADD IF NOT EXISTS is much harder, so I
> haven't done that here.
>
> Additionally, this patch fixes a few minor issues that you had pointed
> out, and merges with the new expression evaluation system in the executor.
Your attachment has a mimetype of invalid/octet-stream - that's a first
for me ;)
Are you going to try to merge this soon, or are you pushing this to 11?
Feels a bit large for 10 at this point.
> + case T_NextValueExpr:
> + {
> + NextValueExpr *nve = (NextValueExpr *) node;
> +
> + scratch.opcode = EEOP_NEXTVALUEEXPR;
> + scratch.d.nextvalueexpr.seqid = nve->seqid;
> +
> + ExprEvalPushStep(state, &scratch);
> + break;
> + }
Hm - that's probably been answered somewhere, but why do we need a
special expression type for this?
> default:
> elog(ERROR, "unrecognized node type: %d",
> (int) nodeTag(node));
> diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
> index 4fbb5c1e74..5935b9ef75 100644
> --- a/src/backend/executor/execExprInterp.c
> +++ b/src/backend/executor/execExprInterp.c
> @@ -60,6 +60,7 @@
>
> #include "access/tuptoaster.h"
> #include "catalog/pg_type.h"
> +#include "commands/sequence.h"
> #include "executor/execExpr.h"
> #include "executor/nodeSubplan.h"
> #include "funcapi.h"
> @@ -337,6 +338,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
> &&CASE_EEOP_NULLIF,
> &&CASE_EEOP_SQLVALUEFUNCTION,
> &&CASE_EEOP_CURRENTOFEXPR,
> + &&CASE_EEOP_NEXTVALUEEXPR,
> &&CASE_EEOP_ARRAYEXPR,
> &&CASE_EEOP_ARRAYCOERCE,
> &&CASE_EEOP_ROW,
> @@ -1228,6 +1230,14 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
> EEO_NEXT();
> }
>
> + EEO_CASE(EEOP_NEXTVALUEEXPR)
> + {
> + *op->resvalue = Int64GetDatum(nextval_internal(op->d.nextvalueexpr.seqid, false));
> + *op->resnull = false;
> +
> + EEO_NEXT();
> + }
Is it guaranteed that the caller expects an int64? I saw that
nextvalueexpr's have a typeid field.
Greetings,
Andres Freund
On 3/30/17 22:57, Vitaly Burovoy wrote: > Why do you still want to leave "ADD IF NOT EXISTS" instead of using > "SET IF NOT EXISTS"? > If someone wants to follow the standard he can simply not to use "IF > NOT EXISTS" at all. Without it error should be raised. As I tried to mention earlier, it is very difficult to implement the IF NOT EXISTS behavior here, because we need to run the commands the create the sequence before we know whether we will need it. So this is a bit different from many other ALTER TABLE commands. It could be done, but it would require some major reworking of things or a new idea of some kind. It could be done later, but I think it's not worth holding things up for that. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 4/3/17 14:19, Andres Freund wrote:
> Are you going to try to merge this soon, or are you pushing this to 11?
I plan to commit it this week. It was basically ready weeks ago, but
had to be adjusted after the executor changes.
>> + case T_NextValueExpr:
>> + {
>> + NextValueExpr *nve = (NextValueExpr *) node;
>> +
>> + scratch.opcode = EEOP_NEXTVALUEEXPR;
>> + scratch.d.nextvalueexpr.seqid = nve->seqid;
>> +
>> + ExprEvalPushStep(state, &scratch);
>> + break;
>> + }
>
> Hm - that's probably been answered somewhere, but why do we need a
> special expression type for this?
Because that's the way to evade the separate permission check that
nextval the function would otherwise do.
>> diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
>> index 4fbb5c1e74..5935b9ef75 100644
>> --- a/src/backend/executor/execExprInterp.c
>> +++ b/src/backend/executor/execExprInterp.c
>> @@ -60,6 +60,7 @@
>>
>> #include "access/tuptoaster.h"
>> #include "catalog/pg_type.h"
>> +#include "commands/sequence.h"
>> #include "executor/execExpr.h"
>> #include "executor/nodeSubplan.h"
>> #include "funcapi.h"
>> @@ -337,6 +338,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
>> &&CASE_EEOP_NULLIF,
>> &&CASE_EEOP_SQLVALUEFUNCTION,
>> &&CASE_EEOP_CURRENTOFEXPR,
>> + &&CASE_EEOP_NEXTVALUEEXPR,
>> &&CASE_EEOP_ARRAYEXPR,
>> &&CASE_EEOP_ARRAYCOERCE,
>> &&CASE_EEOP_ROW,
>> @@ -1228,6 +1230,14 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
>> EEO_NEXT();
>> }
>>
>> + EEO_CASE(EEOP_NEXTVALUEEXPR)
>> + {
>> + *op->resvalue = Int64GetDatum(nextval_internal(op->d.nextvalueexpr.seqid, false));
>> + *op->resnull = false;
>> +
>> + EEO_NEXT();
>> + }
>
> Is it guaranteed that the caller expects an int64? I saw that
> nextvalueexpr's have a typeid field.
It expects one of the integer types. We could cast the result of
Int64GetDatum() to the appropriate type, but that wouldn't actually do
anything.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2017-04-03 16:31:27 -0400, Peter Eisentraut wrote: > > Is it guaranteed that the caller expects an int64? I saw that > > nextvalueexpr's have a typeid field. > > It expects one of the integer types. We could cast the result of > Int64GetDatum() to the appropriate type, but that wouldn't actually do > anything. Uh. What about 32bit systems (and 64bit systems without float8passbyval)? Greetings, Andres Freund
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 4/3/17 14:19, Andres Freund wrote:
> + *op->resvalue = Int64GetDatum(nextval_internal(op->d.nextvalueexpr.seqid, false));
>> Is it guaranteed that the caller expects an int64? I saw that
>> nextvalueexpr's have a typeid field.
> It expects one of the integer types. We could cast the result of
> Int64GetDatum() to the appropriate type, but that wouldn't actually do
> anything.
Uh, really? On 32-bit platforms, int64 and int32 datums have entirely
different representations.
regards, tom lane
On 4/3/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 3/30/17 22:57, Vitaly Burovoy wrote: >> Why do you still want to leave "ADD IF NOT EXISTS" instead of using >> "SET IF NOT EXISTS"? >> If someone wants to follow the standard he can simply not to use "IF >> NOT EXISTS" at all. Without it error should be raised. > > As I tried to mention earlier, it is very difficult to implement the IF > NOT EXISTS behavior here, because we need to run the commands the create > the sequence before we know whether we will need it. I understand. You did not mention the reason. But now you have the "get_attidentity" function to check whether the column is an identity or not. If it is not so create a sequence otherwise skip the creation. I'm afraid after the feature freeze it is impossible to do "major reworking of things", but after the release we have to support the "ALTER COLUMN col ADD" grammar. > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services > The next nitpickings to the last patch. I try to get places with lacking of variables' initialization. All other things seem good for me now. I'll continue to review the patch while you're fixing the current notes. Unfortunately I don't know PG internals so deep to understand correctness of changes in the executor (what Tom and Andres are talking about). 0. There is a little conflict of applying to the current master because of 60a0b2e. 1. First of all, I think the previous usage of the constant "ATTRIBUTE_IDENTITY_NONE" (for setting a value) is better than just '\0'. It is OK for lacking of the constant in comparison. 2. Lack of "SET IDENTITY" and "RESTART" in the "Compatibility" chapter in "alter_table.sgml": "The forms ADD (without USING INDEX), DROP, SET DEFAULT, and SET DATA TYPE (without USING) conform with the SQL standard." Also ADD IDENTITY is an extension (I hope temporary), but it looks like a standard feature (from the above sentance). 3. It would be better if tab-completion has ability to complete the "RESTART" keyword like: ALTER TABLE x1 ALTER COLUMN i RESTART tab-complete.c:8898 - COMPLETE_WITH_LIST5("TYPE", "SET", "RESET", "ADD", "DROP"); + COMPLETE_WITH_LIST5("TYPE", "SET", "RESET", "RESTART", "ADD", "DROP"); 4. The block in "gram.y": /* ALTER TABLE <name> ALTER [COLUMN] <colname> DROP IDENTITY */ does not contain "n->missing_ok = false;". I think it should be. 5. In the file "pg_dump.c" in the "getTableAttrs" function at row 7959 the "tbinfo->needs_override" is used (in the "||" operator) but it is initially nowhere set to "false". 6. And finally... a segfault when pre-9.6 databases are pg_dump-ing: SQL query in the file "pg_dump.c" in funcion "getTables" has the "is_identity_sequence" column only in the "if (fout->remoteVersion >= 90600)" block (frow 5536), whereas 9.6 does not support it (but OK, for 9.6 it is always FALSE, no sense to create a new "if" block), but in other blocks no ",FALSE as is_identity_sequence" is added. The same mistake is in the "getTableAttrs" function. Moreover whether "ORDER BY a.attrelid, a.attnum" is really necessary ("else if (fout->remoteVersion >= 90200)" has only "ORDER BY a.attnum")? -- Best regards, Vitaly Burovoy
On 4/4/17, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote:
> On 4/3/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>> On 3/30/17 22:57, Vitaly Burovoy wrote:
>>> Why do you still want to leave "ADD IF NOT EXISTS" instead of using
>>> "SET IF NOT EXISTS"?
>>> If someone wants to follow the standard he can simply not to use "IF
>>> NOT EXISTS" at all. Without it error should be raised.
>>
>> As I tried to mention earlier, it is very difficult to implement the IF
>> NOT EXISTS behavior here, because we need to run the commands the create
>> the sequence before we know whether we will need it.
>
> I understand. You did not mention the reason.
> But now you have the "get_attidentity" function to check whether the
> column is an identity or not.
> If it is not so create a sequence otherwise skip the creation.
>
> I'm afraid after the feature freeze it is impossible to do "major
> reworking of things", but after the release we have to support the
> "ALTER COLUMN col ADD" grammar.
So it would be great if you have a time to get rid of "ALTER ... ADD
GENERATED" syntax in favor of "ALTER ... SET GENERATED ... IF NOT
EXIST"
> 3.
> It would be better if tab-completion has ability to complete the
> "RESTART" keyword like:
> ALTER TABLE x1 ALTER COLUMN i RESTART
> tab-complete.c:8898
> - COMPLETE_WITH_LIST5("TYPE", "SET", "RESET", "ADD", "DROP");
> + COMPLETE_WITH_LIST5("TYPE", "SET", "RESET", "RESTART", "ADD", "DROP");
Oops. Of course
+ COMPLETE_WITH_LIST6("TYPE", "SET", "RESET", "RESTART", "ADD", "DROP");
--
Best regards,
Vitaly Burovoy
On 4/4/17 22:53, Vitaly Burovoy wrote:
> The next nitpickings to the last patch. I try to get places with
> lacking of variables' initialization.
> All other things seem good for me now. I'll continue to review the
> patch while you're fixing the current notes.
Committed with your changes (see below) as well as executor fix.
> 1.
> First of all, I think the previous usage of the constant
> "ATTRIBUTE_IDENTITY_NONE" (for setting a value) is better than just
> '\0'.
> It is OK for lacking of the constant in comparison.
That required adding pg_attribute.h to too many places, so I took it out.
> 2.
> Lack of "SET IDENTITY" and "RESTART" in the "Compatibility" chapter in
> "alter_table.sgml":
> "The forms ADD (without USING INDEX), DROP, SET DEFAULT, and SET DATA
> TYPE (without USING) conform with the SQL standard."
> Also ADD IDENTITY is an extension (I hope temporary), but it looks
> like a standard feature (from the above sentance).
added that
> 3.
> It would be better if tab-completion has ability to complete the
> "RESTART" keyword like:
> ALTER TABLE x1 ALTER COLUMN i RESTART
> tab-complete.c:8898
> - COMPLETE_WITH_LIST5("TYPE", "SET", "RESET", "ADD", "DROP");
> + COMPLETE_WITH_LIST5("TYPE", "SET", "RESET", "RESTART", "ADD", "DROP");
done
> 4.
> The block in "gram.y":
> /* ALTER TABLE <name> ALTER [COLUMN] <colname> DROP IDENTITY */
> does not contain "n->missing_ok = false;". I think it should be.
done
> 5.
> In the file "pg_dump.c" in the "getTableAttrs" function at row 7959
> the "tbinfo->needs_override" is used (in the "||" operator) but it is
> initially nowhere set to "false".
The structure is initialized to zero. Not all fields are explicitly
initialized.
> 6.
> And finally... a segfault when pre-9.6 databases are pg_dump-ing:
> SQL query in the file "pg_dump.c" in funcion "getTables" has the
> "is_identity_sequence" column only in the "if (fout->remoteVersion >=
> 90600)" block (frow 5536), whereas 9.6 does not support it (but OK,
> for 9.6 it is always FALSE, no sense to create a new "if" block), but
> in other blocks no ",FALSE as is_identity_sequence" is added.
>
> The same mistake is in the "getTableAttrs" function. Moreover whether
> "ORDER BY a.attrelid, a.attnum" is really necessary ("else if
> (fout->remoteVersion >= 90200)" has only "ORDER BY a.attnum")?
fixed
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 4/6/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 4/4/17 22:53, Vitaly Burovoy wrote: >> The next nitpickings to the last patch. I try to get places with >> lacking of variables' initialization. >> All other things seem good for me now. I'll continue to review the >> patch while you're fixing the current notes. > > Committed with your changes (see below) as well as executor fix. Thank you very much! > As I tried to mention earlier, it is very difficult to implement the IF > NOT EXISTS behavior here, because we need to run the commands the create > the sequence before we know whether we will need it. In fact with the function "get_attidentity" it is not so hard. Please, find the patch attached. I've implement SET GENERATED ... IF NOT EXISTS. It must be placed before other SET options but fortunately it conforms with the standard. Since that form always changes the sequence behind the column, I decided to explicitly write "[NO] CACHE" in pg_dump. As a plus now it is possible to rename the sequence behind the column by specifying SEQUENCE NAME in SET GENERATED. I hope it is still possible to get rid of the "ADD GENERATED" syntax. > > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Best regards, Vitaly Burovoy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 4/6/17, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote: > On 4/6/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: >> As I tried to mention earlier, it is very difficult to implement the IF >> NOT EXISTS behavior here, because we need to run the commands the create >> the sequence before we know whether we will need it. > > In fact with the function "get_attidentity" it is not so hard. Please, > find the patch attached. ... > I hope it is still possible to get rid of the "ADD GENERATED" syntax. Hello hackers, Is it possible to commit the patch from the previous letter? It was sent before the feature freeze, introduces no new feature (only syntax change discussed in this thread and not implemented due to lack of a time of the author) and can be considered as a fix to the committed patch (similar to the second round in "sequence data type"). -- Best regards, Vitaly Burovoy
On Thu, Apr 13, 2017 at 7:40 PM, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote: > On 4/6/17, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote: >> On 4/6/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: >>> As I tried to mention earlier, it is very difficult to implement the IF >>> NOT EXISTS behavior here, because we need to run the commands the create >>> the sequence before we know whether we will need it. >> >> In fact with the function "get_attidentity" it is not so hard. Please, >> find the patch attached. > ... >> I hope it is still possible to get rid of the "ADD GENERATED" syntax. > > Is it possible to commit the patch from the previous letter? > It was sent before the feature freeze, introduces no new feature (only > syntax change discussed in this thread and not implemented due to lack > of a time of the author) and can be considered as a fix to the > committed patch (similar to the second round in "sequence data type"). It seems to me that at least the part about removing ADD GENERATED could be applied as an adjustment for the committed patch (INE would be a new feature), and if there is a time to adjust already committed features for the upcoming release, it is now. So I have added an open item. -- Michael
On Thu, Apr 06, 2017 at 10:26:16PM -0700, Vitaly Burovoy wrote: > On 4/6/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > On 4/4/17 22:53, Vitaly Burovoy wrote: > >> The next nitpickings to the last patch. I try to get places with > >> lacking of variables' initialization. > >> All other things seem good for me now. I'll continue to review the > >> patch while you're fixing the current notes. > > > > Committed with your changes (see below) as well as executor fix. > > Thank you very much! > > > > As I tried to mention earlier, it is very difficult to implement the IF > > NOT EXISTS behavior here, because we need to run the commands the create > > the sequence before we know whether we will need it. > > In fact with the function "get_attidentity" it is not so hard. Please, > find the patch attached. > I've implement SET GENERATED ... IF NOT EXISTS. It must be placed > before other SET options but fortunately it conforms with the > standard. > Since that form always changes the sequence behind the column, I > decided to explicitly write "[NO] CACHE" in pg_dump. > > As a plus now it is possible to rename the sequence behind the column > by specifying SEQUENCE NAME in SET GENERATED. > > I hope it is still possible to get rid of the "ADD GENERATED" syntax. [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Peter, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On Fri, Apr 14, 2017 at 05:56:44AM +0000, Noah Misch wrote: > On Thu, Apr 06, 2017 at 10:26:16PM -0700, Vitaly Burovoy wrote: > > On 4/6/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > > On 4/4/17 22:53, Vitaly Burovoy wrote: > > >> The next nitpickings to the last patch. I try to get places with > > >> lacking of variables' initialization. > > >> All other things seem good for me now. I'll continue to review the > > >> patch while you're fixing the current notes. > > > > > > Committed with your changes (see below) as well as executor fix. > > > > Thank you very much! > > > > > > > As I tried to mention earlier, it is very difficult to implement the IF > > > NOT EXISTS behavior here, because we need to run the commands the create > > > the sequence before we know whether we will need it. > > > > In fact with the function "get_attidentity" it is not so hard. Please, > > find the patch attached. > > I've implement SET GENERATED ... IF NOT EXISTS. It must be placed > > before other SET options but fortunately it conforms with the > > standard. > > Since that form always changes the sequence behind the column, I > > decided to explicitly write "[NO] CACHE" in pg_dump. > > > > As a plus now it is possible to rename the sequence behind the column > > by specifying SEQUENCE NAME in SET GENERATED. > > > > I hope it is still possible to get rid of the "ADD GENERATED" syntax. > > [Action required within three days. This is a generic notification.] > > The above-described topic is currently a PostgreSQL 10 open item. Peter, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > v10 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within three calendar days of > this message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping v10. Consequently, I will appreciate your efforts > toward speedy resolution. Thanks. > > [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On 4/7/17 01:26, Vitaly Burovoy wrote: > I've implement SET GENERATED ... IF NOT EXISTS. It must be placed > before other SET options but fortunately it conforms with the > standard. > Since that form always changes the sequence behind the column, I > decided to explicitly write "[NO] CACHE" in pg_dump. > > As a plus now it is possible to rename the sequence behind the column > by specifying SEQUENCE NAME in SET GENERATED. > > I hope it is still possible to get rid of the "ADD GENERATED" syntax. I am still not fond of this change. There is precedent all over the place for having separate commands for creating a structure, changing a structure, and removing a structure. I don't understand what the problem with that is. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 4/18/17 02:07, Noah Misch wrote: > This PostgreSQL 10 open item is past due for your status update. Kindly send > a status update within 24 hours, and include a date for your subsequent status > update. Refer to the policy on open item ownership: > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com I am in disagreement with the submitter that this is a problem or what the solution should be. The discussion is ongoing. Note that the current state isn't actually broken, so it doesn't have to hold anything up. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 4/18/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 4/7/17 01:26, Vitaly Burovoy wrote: >> I've implement SET GENERATED ... IF NOT EXISTS. It must be placed >> before other SET options but fortunately it conforms with the >> standard. >> Since that form always changes the sequence behind the column, I >> decided to explicitly write "[NO] CACHE" in pg_dump. >> >> As a plus now it is possible to rename the sequence behind the column >> by specifying SEQUENCE NAME in SET GENERATED. >> >> I hope it is still possible to get rid of the "ADD GENERATED" syntax. > > I am still not fond of this change. There is precedent all over the > place for having separate commands for creating a structure, changing a > structure, and removing a structure. I don't understand what the > problem with that is. > > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services OK. Let's go through it again. IDENTITY is a property of a column. There are no syntax to change any property of any DB object via the "ADD" syntax. Yes, a structure (a sequence) is created. But in fact it cannot be independent from the column at all (I remind you that according to the standard it should be unnamed sequence and there are really no way to do something with it but via the column's DDL). It is even hard to detect which sequence (since they have names) is owned by the column: postgres=# CREATE TABLE xxx(i int generated always as identity, j serial); CREATE TABLE postgres=# \d xxx* Table "public.xxx"Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------------------------------i | integer | | not null | generatedalways as identityj | integer | | not null | nextval('xxx_j_seq'::regclass) Sequence "public.xxx_i_seq" Column | Type | Value ------------+---------+-------last_value | bigint | 1log_cnt | bigint | 0is_called | boolean | f Sequence "public.xxx_j_seq" Column | Type | Value ------------+---------+-------last_value | bigint | 1log_cnt | bigint | 0is_called | boolean | f Owned by: public.xxx.j I can only guess that "public.xxx_i_seq" is owned by "public.xxx.i", nothing proves that. Whereas for regular sequence there are two evidences ("Default" and "Owned by"). Also the created sequence cannot be deleted (only with the column) or left after the column is deleted. Everywhere else the "ADD" syntax is used where you can add more than one object to the altered one: ALTER TABLE ... ADD COLUMN /* there can be many added columns differing by names in the table */ ALTER TABLE ... ADD CONSTRAINT /* there can be many added constraints differing by names in the table */ ALTER TYPE ... ADD VALUE /* many values in an enum differing by names in the enum */ ALTER TYPE ... ADD ATTRIBUTE /* many attributes in a composite differing by names in the enum */ etc. But what is for "ALTER TABLE ... ALTER COLUMN ... ADD GENERATED"? Whether a property's name is used for a distinction between them in a column? Whether it is possible to have more than one such property to the altering column? The "SET GENERATED" (without "IF NOT EXISTS") syntax conforms to the standard for those who want it. The "SET GENERATED ... IF NOT EXISTS" syntax allows users to have the column be in a required state (IDENTITY with set options) without paying attention whether it is already set as IDENTITY or not. The "[ NOT ] EXISTS" is a common Postgres' syntax extension for creating/updating objects in many places. That's why I think it should be used instead of introducing the new "ADD" syntax which contradicts the users' current experience. -- Best regards, Vitaly Burovoy
On Tue, Apr 18, 2017 at 11:53:36AM -0400, Peter Eisentraut wrote: > On 4/18/17 02:07, Noah Misch wrote: > > This PostgreSQL 10 open item is past due for your status update. Kindly send > > a status update within 24 hours, and include a date for your subsequent status > > update. Refer to the policy on open item ownership: > > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > I am in disagreement with the submitter that this is a problem or what > the solution should be. The discussion is ongoing. Note that the > current state isn't actually broken, so it doesn't have to hold anything up. I see. If anyone in addition to the submitter thinks making a change in this area qualifies as a PostgreSQL 10 open item, please speak up. Otherwise, on Monday, I'll reclassify this as a non-bug.
On Thu, Apr 20, 2017 at 12:05 AM, Vitaly Burovoy
<vitaly.burovoy@gmail.com> wrote:
>> I am still not fond of this change. There is precedent all over the
>> place for having separate commands for creating a structure, changing a
>> structure, and removing a structure. I don't understand what the
>> problem with that is.
I agree. That's not intrinsically a problem.
> OK. Let's go through it again.
> IDENTITY is a property of a column. There are no syntax to change any
> property of any DB object via the "ADD" syntax.
> Yes, a structure (a sequence) is created. But in fact it cannot be
> independent from the column at all (I remind you that according to the
> standard it should be unnamed sequence and there are really no way to
> do something with it but via the column's DDL).
I agree that ADD is a little odd here, but it doesn't seem terrible.
But why do we need it? Instead of:
ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
SET GENERATED { ALWAYS | BY DEFAULT }
DROP IDENTITY [ IF EXISTS ]
Why not just:
SET GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
DROP IDENTITY [ IF EXISTS ]
Surely the ALTER TABLE command can tell whether the column is already
GENERATED, so the first form could make it generated if it's not and
adjust the ALWAYS/BY DEFAULT property if it is.
> It is even hard to detect which sequence (since they have names) is
> owned by the column:
>
> postgres=# CREATE TABLE xxx(i int generated always as identity, j serial);
> CREATE TABLE
> postgres=# \d xxx*
> Table "public.xxx"
> Column | Type | Collation | Nullable | Default
> --------+---------+-----------+----------+--------------------------------
> i | integer | | not null | generated always as identity
> j | integer | | not null | nextval('xxx_j_seq'::regclass)
>
> Sequence "public.xxx_i_seq"
> Column | Type | Value
> ------------+---------+-------
> last_value | bigint | 1
> log_cnt | bigint | 0
> is_called | boolean | f
>
> Sequence "public.xxx_j_seq"
> Column | Type | Value
> ------------+---------+-------
> last_value | bigint | 1
> log_cnt | bigint | 0
> is_called | boolean | f
> Owned by: public.xxx.j
>
> I can only guess that "public.xxx_i_seq" is owned by "public.xxx.i",
> nothing proves that.
> Whereas for regular sequence there are two evidences ("Default" and "Owned by").
This seems like a psql deficiency that should be fixed.
> Also the created sequence cannot be deleted (only with the column) or
> left after the column is deleted.
This does not seem like a problem. In fact I'd say that's exactly the
desirable behavior.
> The "[ NOT ] EXISTS" is a common Postgres' syntax extension for
> creating/updating objects in many places. That's why I think it should
> be used instead of introducing the new "ADD" syntax which contradicts
> the users' current experience.
As noted above, I don't understand why we need either ADD or IF NOT
EXISTS. Why can't SET just, eh, set the property to the desired
state?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 4/23/17, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Apr 20, 2017 at 12:05 AM, Vitaly Burovoy
> <vitaly.burovoy@gmail.com> wrote:
>> OK. Let's go through it again.
>> IDENTITY is a property of a column. There are no syntax to change any
>> property of any DB object via the "ADD" syntax.
>> Yes, a structure (a sequence) is created. But in fact it cannot be
>> independent from the column at all (I remind you that according to the
>> standard it should be unnamed sequence and there are really no way to
>> do something with it but via the column's DDL).
>
> I agree that ADD is a little odd here, but it doesn't seem terrible.
Yes, it is not terrible, but why do we need to support an odd syntax
if we can use a correct one?
If we leave it as it was committed, we have to support it for years.
> But why do we need it? Instead of:
>
> ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
> SET GENERATED { ALWAYS | BY DEFAULT }
> DROP IDENTITY [ IF EXISTS ]
>
> Why not just:
>
> SET GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
> DROP IDENTITY [ IF EXISTS ]
>
> Surely the ALTER TABLE command can tell whether the column is already
> GENERATED, so the first form could make it generated if it's not and
> adjust the ALWAYS/BY DEFAULT property if it is.
I thought exactly that way, but Peter gave an explanation[1].
I had to search a different way because no one joined to the
discussion at that time.
One of reasons from Peter was to make "SET GENERATED" follow the
standard (i.e. raise an error).
I asked whether "IF NOT EXISTS" works for him instead of "ADD GENERATED".
The answer[2] was "It could be done", but "it is very difficult to implement".
So I wonder why the adjustment patch is not wished for being committed.
>> It is even hard to detect which sequence (since they have names) is
>> owned by the column:
>>
>> postgres=# CREATE TABLE xxx(i int generated always as identity, j
>> serial);
>> CREATE TABLE
>> postgres=# \d xxx*
>> Table "public.xxx"
>> Column | Type | Collation | Nullable | Default
>> --------+---------+-----------+----------+--------------------------------
>> i | integer | | not null | generated always as identity
>> j | integer | | not null | nextval('xxx_j_seq'::regclass)
>>
>> Sequence "public.xxx_i_seq"
>> Column | Type | Value
>> ------------+---------+-------
>> last_value | bigint | 1
>> log_cnt | bigint | 0
>> is_called | boolean | f
>>
>> Sequence "public.xxx_j_seq"
>> Column | Type | Value
>> ------------+---------+-------
>> last_value | bigint | 1
>> log_cnt | bigint | 0
>> is_called | boolean | f
>> Owned by: public.xxx.j
>>
>> I can only guess that "public.xxx_i_seq" is owned by "public.xxx.i",
>> nothing proves that.
>> Whereas for regular sequence there are two evidences ("Default" and "Owned
>> by").
>
> This seems like a psql deficiency that should be fixed.
It was a part of explanation why IDENTITY is a property and therefore
"SET" should be used instead of "ADD".
>> Also the created sequence cannot be deleted (only with the column) or
>> left after the column is deleted.
>
> This does not seem like a problem. In fact I'd say that's exactly the
> desirable behavior.
Also it is not about a problem, it is a part of explanation.
>> The "[ NOT ] EXISTS" is a common Postgres' syntax extension for
>> creating/updating objects in many places. That's why I think it should
>> be used instead of introducing the new "ADD" syntax which contradicts
>> the users' current experience.
>
> As noted above, I don't understand why we need either ADD or IF NOT
> EXISTS. Why can't SET just, eh, set the property to the desired
> state?
+1
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
[1] https://postgr.es/m/497c40af-bd7a-5cb3-d028-d91434639fe0@2ndquadrant.com
[2] https://postgr.es/m/59d8e32a-14de-6f45-c2b0-bb52e4a7522d@2ndquadrant.com
--
Best regards,
Vitaly Burovoy
On Mon, Apr 24, 2017 at 10:03 AM, Vitaly Burovoy
<vitaly.burovoy@gmail.com> wrote:
> On 4/23/17, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Thu, Apr 20, 2017 at 12:05 AM, Vitaly Burovoy
>> <vitaly.burovoy@gmail.com> wrote:
>> But why do we need it? Instead of:
>>
>> ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
>> SET GENERATED { ALWAYS | BY DEFAULT }
>> DROP IDENTITY [ IF EXISTS ]
>>
>> Why not just:
>>
>> SET GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
>> DROP IDENTITY [ IF EXISTS ]
>>
>> Surely the ALTER TABLE command can tell whether the column is already
>> GENERATED, so the first form could make it generated if it's not and
>> adjust the ALWAYS/BY DEFAULT property if it is.
>
> I thought exactly that way, but Peter gave an explanation[1].
> I had to search a different way because no one joined to the
> discussion at that time.
> One of reasons from Peter was to make "SET GENERATED" follow the
> standard (i.e. raise an error).
> I asked whether "IF NOT EXISTS" works for him instead of "ADD GENERATED".
> The answer[2] was "It could be done", but "it is very difficult to implement".
>
> So I wonder why the adjustment patch is not wished for being committed.
Same line of thoughts here, as far as I understand, ADD GENERATED and
SET GENERATED have a lot in common, SET GENERATED follows the SQL
spec, and not ADD GENERATED, so I don't have a good reason to not
simplify the interface by keeping SET GENERATED and dropping ADD. This
will be less confusing to users.
--
Michael
On Sun, Apr 23, 2017 at 9:03 PM, Vitaly Burovoy
<vitaly.burovoy@gmail.com> wrote:
>> But why do we need it? Instead of:
>>
>> ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
>> SET GENERATED { ALWAYS | BY DEFAULT }
>> DROP IDENTITY [ IF EXISTS ]
>>
>> Why not just:
>>
>> SET GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
>> DROP IDENTITY [ IF EXISTS ]
>>
>> Surely the ALTER TABLE command can tell whether the column is already
>> GENERATED, so the first form could make it generated if it's not and
>> adjust the ALWAYS/BY DEFAULT property if it is.
>
> I thought exactly that way, but Peter gave an explanation[1].
That's not really an explanation. Peter says he needs ADD to make
pg_dump, but he doesn't really. He just needs something that adds it,
and augmenting SET to perform ADD if the sequence is not currently
GENERATED would be fine.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 4/23/17 16:58, Robert Haas wrote:
> I agree that ADD is a little odd here, but it doesn't seem terrible.
> But why do we need it? Instead of:
>
> ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
> SET GENERATED { ALWAYS | BY DEFAULT }
> DROP IDENTITY [ IF EXISTS ]
>
> Why not just:
>
> SET GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
> DROP IDENTITY [ IF EXISTS ]
>
> Surely the ALTER TABLE command can tell whether the column is already
> GENERATED, so the first form could make it generated if it's not and
> adjust the ALWAYS/BY DEFAULT property if it is.
Note that DROP IDENTITY is a non-idempotent command (i.e., it errors if
the thing has already been dropped), per SQL standard, which is why we
have IF EXISTS there. So it would be weird if the corresponding
creation command would be idempotent (i.e., it did not care whether the
thing is already there).
Also, if we tried to merge the ADD and SET cases, the syntax would come
out weird. The creation syntax is
CREATE TABLE t1 (c1 int GENERATED ALWAYS AS IDENTITY);
The syntax to change an existing column is
ALTER TABLE t1 ALTER COLUMN c1 SET GENERATED ALWAYS;
But we can't just make the "AS IDENTITY" optional, because that same
syntax is also used by the "generated columns" feature.
So we could make up new syntax
ALTER TABLE t1 ALTER COLUMN c1 SET GENERATED ALWAYS AS IDENTITY;
and let that be set-or-add, but then the argument for staying within the
SQL standard goes out the window.
Finally, I had mentioned that earlier in this thread, the behavior of
the sequence options differs depending on whether the sequence already
exists. So if you wrote
ALTER TABLE t1 ALTER COLUMN c1 SET GENERATED ALWAYS AS IDENTITY (START 2);
and the sequence does not exist, you get a new sequence with START 2 and
all default values otherwise. If the sequence already exists, you keep
the sequence and just change the start value. So that's not truly
idempotent either.
So I think altogether it is much clearer and more consistent to have
separate verbs for create/change/remove.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Apr 26, 2017 at 10:03 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 4/23/17 16:58, Robert Haas wrote:
>> I agree that ADD is a little odd here, but it doesn't seem terrible.
>> But why do we need it? Instead of:
>>
>> ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
>> SET GENERATED { ALWAYS | BY DEFAULT }
>> DROP IDENTITY [ IF EXISTS ]
>>
>> Why not just:
>>
>> SET GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
>> DROP IDENTITY [ IF EXISTS ]
>>
>> Surely the ALTER TABLE command can tell whether the column is already
>> GENERATED, so the first form could make it generated if it's not and
>> adjust the ALWAYS/BY DEFAULT property if it is.
>
> Note that DROP IDENTITY is a non-idempotent command (i.e., it errors if
> the thing has already been dropped), per SQL standard, which is why we
> have IF EXISTS there. So it would be weird if the corresponding
> creation command would be idempotent (i.e., it did not care whether the
> thing is already there).
Hmm, I guess that has some validity to it.
> Also, if we tried to merge the ADD and SET cases, the syntax would come
> out weird. The creation syntax is
>
> CREATE TABLE t1 (c1 int GENERATED ALWAYS AS IDENTITY);
>
> The syntax to change an existing column is
>
> ALTER TABLE t1 ALTER COLUMN c1 SET GENERATED ALWAYS;
>
> But we can't just make the "AS IDENTITY" optional, because that same
> syntax is also used by the "generated columns" feature.
I don't understand how that follows.
> So we could make up new syntax
>
> ALTER TABLE t1 ALTER COLUMN c1 SET GENERATED ALWAYS AS IDENTITY;
>
> and let that be set-or-add, but then the argument for staying within the
> SQL standard goes out the window.
What does the SQL standard actually mandate here? It's not clear to
me which parts of this syntax are mandated by the standard and which
we just made up. I'm gathering (perhaps incorrectly) that you made
ALTER TABLE ... DROP IDENTITY as per standard, but the reverse
operation of setting the identity non-standard syntax. If that's so,
it seems like a questionable choice.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 4/27/17 10:03, Robert Haas wrote: >> So we could make up new syntax >> >> ALTER TABLE t1 ALTER COLUMN c1 SET GENERATED ALWAYS AS IDENTITY; >> >> and let that be set-or-add, but then the argument for staying within the >> SQL standard goes out the window. > > What does the SQL standard actually mandate here? It's not clear to > me which parts of this syntax are mandated by the standard and which > we just made up. I'm gathering (perhaps incorrectly) that you made > ALTER TABLE ... DROP IDENTITY as per standard, but the reverse > operation of setting the identity non-standard syntax. If that's so, > it seems like a questionable choice. Standard syntax: 1. Add new column with identity: ALTER TABLE t1 ADD COLUMN c1 int GENERATED ALWAYS AS IDENTITY; (or equivalent for CREATE TABLE) 2. Change ALWAYS to BY DEFAULT (or vice versa) of existing column: ALTER TABLE t1 ALTER COLUMN c1 SET GENERATED BY DEFAULT; 3. Change sequence parameters of existing column: ALTER TABLE t1 ALTER COLUMN c1 SET (START 2) (the previous two can be combined) 4. Drop identity property of existing column: ALTER TABLE t1 ALTER COLUMN c1 DROP IDENTITY; (with IF EXISTS being our extension) There is no standard syntax for the inverse, that is, adding an identity property to an existing column. (I have checked DB2 and Oracle. They don't have anything either. One even documents that explicitly.) Therefore ... Nonstandard syntax: 5. Add identity property to existing column: ALTER TABLE t1 ALTER COLUMN c1 ADD GENERATED ALWAYS AS IDENTITY; The competing proposal is that we should not have syntax #5 but that syntax #2 should (also) do that. My concerns about that, as previously explained, are - asymmetric idempotency behavior with DROP IDENTITY - ambiguous/inconsistent behavior when sequence options are specified in same command - syntax ambiguity/inconsistency with other SQL standard features The argument in favor is that syntax #5 is nonstandard. But of course making #2 doing something nonstandard is also nonstandard. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Apr 27, 2017 at 3:42 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 4/27/17 10:03, Robert Haas wrote: >>> So we could make up new syntax >>> >>> ALTER TABLE t1 ALTER COLUMN c1 SET GENERATED ALWAYS AS IDENTITY; >>> >>> and let that be set-or-add, but then the argument for staying within the >>> SQL standard goes out the window. >> >> What does the SQL standard actually mandate here? It's not clear to >> me which parts of this syntax are mandated by the standard and which >> we just made up. I'm gathering (perhaps incorrectly) that you made >> ALTER TABLE ... DROP IDENTITY as per standard, but the reverse >> operation of setting the identity non-standard syntax. If that's so, >> it seems like a questionable choice. > > Standard syntax: > > 1. Add new column with identity: > > ALTER TABLE t1 ADD COLUMN c1 int GENERATED ALWAYS AS IDENTITY; > > (or equivalent for CREATE TABLE) > > 2. Change ALWAYS to BY DEFAULT (or vice versa) of existing column: > > ALTER TABLE t1 ALTER COLUMN c1 SET GENERATED BY DEFAULT; > > 3. Change sequence parameters of existing column: > > ALTER TABLE t1 ALTER COLUMN c1 SET (START 2) > > (the previous two can be combined) > > 4. Drop identity property of existing column: > > ALTER TABLE t1 ALTER COLUMN c1 DROP IDENTITY; > > (with IF EXISTS being our extension) > > There is no standard syntax for the inverse, that is, adding an identity > property to an existing column. (I have checked DB2 and Oracle. They > don't have anything either. One even documents that explicitly.) > Therefore ... > > Nonstandard syntax: > > 5. Add identity property to existing column: > > ALTER TABLE t1 ALTER COLUMN c1 ADD GENERATED ALWAYS AS IDENTITY; > > The competing proposal is that we should not have syntax #5 but that > syntax #2 should (also) do that. > > My concerns about that, as previously explained, are > > - asymmetric idempotency behavior with DROP IDENTITY > > - ambiguous/inconsistent behavior when sequence options are specified in > same command > > - syntax ambiguity/inconsistency with other SQL standard features > > The argument in favor is that syntax #5 is nonstandard. But of course > making #2 doing something nonstandard is also nonstandard. OK, got it. Given that explanation, I'm not really prepared to argue this matter further. That sounds basically reasonable, even if somebody else might've chosen to do things differently. I still think you should consider improving the psql output, though. Vitaly's examples upthread indicate that for a serial sequence, there's psql output showing the linkage between the table and sequence in both directions, but not when GENERATED is used. Can we get something morally equivalent for the GENERATED case? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 4/27/17 16:10, Robert Haas wrote: > I still think you should consider improving the psql output, though. > Vitaly's examples upthread indicate that for a serial sequence, > there's psql output showing the linkage between the table and sequence > in both directions, but not when GENERATED is used. Can we get > something morally equivalent for the GENERATED case? Done. Good catch. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Apr 28, 2017 at 2:49 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 4/27/17 16:10, Robert Haas wrote: >> I still think you should consider improving the psql output, though. >> Vitaly's examples upthread indicate that for a serial sequence, >> there's psql output showing the linkage between the table and sequence >> in both directions, but not when GENERATED is used. Can we get >> something morally equivalent for the GENERATED case? > > Done. Good catch. Cool. Thanks. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company