On 4/3/17, Peter Eisentraut <peter.eisentr...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
