identity columns

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
53 messages Options
123
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

identity columns

Peter Eisentraut-6
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@...
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@...
https://www.postgresql.org/message-id/flat/45DACD1E.2000207%40dunaweb.hu#45DACD1E.2000207@...
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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

identity-columns.patch (98K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Vitaly Burovoy
Hello,

The first look at the patch:

On 8/30/16, Peter Eisentraut <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

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 while processing 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 always as identity
 b      | 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 identity
 b      | 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Peter Eisentraut-6
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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Vitaly Burovoy
On 9/12/16, Peter Eisentraut <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Robert Haas
In reply to this post by Peter Eisentraut-6
On Mon, Sep 12, 2016 at 5:02 PM, Peter Eisentraut
<[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Peter Eisentraut-6
In reply to this post by Vitaly Burovoy
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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

v2-identity-columns.patch (127K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Haribabu Kommi-2
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Haribabu Kommi-2


On Tue, Nov 22, 2016 at 10:51 PM, Haribabu Kommi <[hidden email]> 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 smoother
operation of commitfest.

Please Ignore if you already shared your review.

Moved to next CF with "needs review" status.


Regards,
Hari Babu
Fujitsu Australia
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Peter Eisentraut-6
In reply to this post by Peter Eisentraut-6
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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

v3-0001-Identity-columns.patch (134K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Vitaly Burovoy
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 |           |          | generated always as identity
 n1     | 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Michael Paquier
On Thu, Jan 5, 2017 at 9:34 AM, Vitaly Burovoy <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Peter Eisentraut-6
In reply to this post by Vitaly Burovoy
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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

v4-0001-Identity-columns.patch (148K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Peter Eisentraut-6
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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Vitaly Burovoy
On 3/15/17, Peter Eisentraut <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Vitaly Burovoy
In reply to this post by Peter Eisentraut-6
On 2/28/17, Peter Eisentraut <[hidden email]> 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  | 7
 log_cnt    | bigint  | 0
 is_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 | generated by default as identity
 b      | 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 relations
 Schema |     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 | generated by default as identity
 b      | 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Peter Eisentraut-6
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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

v5-0001-Identity-columns.patch (206K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Vitaly Burovoy
I haven't seen a patch (I'll do it later), just few notes:

On 3/21/17, Peter Eisentraut <[hidden email]> 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@...
[2] https://www.postgresql.org/docs/current/static/sql-altertable.html
--
Best regards,
Vitaly Burovoy


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Peter Eisentraut-6
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 you could 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: identity columns

Vitaly Burovoy
On 3/21/17, Peter Eisentraut <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
123
Previous Thread Next Thread
Loading...