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
