Re: [HACKERS] identity columns - Mailing list pgsql-hackers

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