Re: identity columns

On 4/3/17, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)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