Hello, The first look at the patch:
On 8/30/16, Peter Eisentraut <peter.eisentr...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers