Обсуждение: SQL2003 GENERATED ... AS ... syntax

As previously mentioned, I'm working on implementing [subject].  I
think I've mostly worked it out, but I'm having trouble with the
GENERATED ALWAYS case.

My changes (see attached patch) can be summarized as follows:

 - in backend/utils/adt/misc.c:

   - added a force_default_value() function which takes a string
     argument (the name of the column to force to default) and
     currently does nothing.

 - in backend/parser/gram.y:

   - when GENERATED ... AS ... is encountered in a column definition,
     it adds a node of the new T_Generated type to the constraint
     list.  This node contains a bool that differentiates between BY
     DEFAULT and ALWAYS, and a pointer to a CreateSeqStmt (for
     IDENTITY '(' OptSeqList ')') or a List constructed by the a_expr
     production (for '(' a_expr ')')

 - in backend/parser/analyze.c:

   - factored out the code from transformColumnDefinition() that
     creates the sequence and the DEFAULT constraint into a separate
     CreateSerialColumn() function which takes as one of its arguments
     is a List of sequence options.  The SERIAL code passes in a NIL
     list, while the GENERATED AS IDENTITY code passes in the option
     list from the CreateSeqStmt.

   - added a CreateAlwaysDefaultColumn() function which synthesizes a
     CreateTrigStmt equivalent to CREATE TRIGGER foo BEFORE INSERT ON
     bar FOR EACH ROW EXECUTE PROCEDURE force_default_value ('baz')
     and adds it to the work list.  This function is called by
     transformColumnDefinition() if a Generated node with always set
     to true is encountered.

Now I must be doing something wrong in CreateAlwaysDefaultColumn(),
because the CreateTrigStmt fails to execute:

| des=# create table test ( id int generated always as identity ( minvalue 1000 ), word text );
| NOTICE:  CREATE TABLE will create implicit sequence "test_id_seq" for SERIAL column "test.id"
| NOTICE:  CREATE TABLE will create implicit trigger "test_id_always_default" for column "test.id"
| ERROR:  relation "public.test" does not exist

GENERATED BY DEFAULT AS IDENTITY works fine though, so I must have
done *something* right:

| des=# create table test ( id int generated by default as identity ( minvalue 1000 ), word text );
| NOTICE:  CREATE TABLE will create implicit sequence "test_id_seq" for SERIAL column "test.id"
| CREATE TABLE
| des=# select sequence_name, last_value, min_value, max_value from test_id_seq;
|  sequence_name | last_value | min_value |      max_value
| ---------------+------------+-----------+---------------------
|  test_id_seq   |       1000 |      1000 | 9223372036854775807
| (1 row)
|

On the other hand, I seem to have botched the definition of
force_default_value() in include/catalog/pg_proc.h, because adding the
trigger manually doesn't seem to work either:

| des=# \df force_default_value
|                              List of functions
|  Result data type |   Schema   |        Name         | Argument data types
| ------------------+------------+---------------------+---------------------
|  "trigger"        | pg_catalog | force_default_value | text
| (1 row)
|
| des=# create trigger test_id_always_default before insert on test for each row execute procedure force_default_value
('id');
| ERROR:  function force_default_value() does not exist

Any suggestions?

DES
--
Dag-Erling Smørgrav - des@des.no

> On the other hand, I seem to have botched the definition of
> force_default_value() in include/catalog/pg_proc.h, because adding the
> trigger manually doesn't seem to work either:
>
> | des=# \df force_default_value
> |                              List of functions
> |  Result data type |   Schema   |        Name         | Argument data types
> | ------------------+------------+---------------------+---------------------
> |  "trigger"        | pg_catalog | force_default_value | text
> | (1 row)
> |
> | des=# create trigger test_id_always_default before insert on test for each row execute procedure
force_default_value('id'); 
> | ERROR:  function force_default_value() does not exist
>
> Any suggestions?

Triggers are strange this way.  You need to create the function without
any arguments. The procedure is expected to find (and enforce) the
arguments through use of the TriggerData struct.

ttdummy() in src/test/regress/regress.c is an example of a Trigger
taking 2 arguments.


Using triggers could cause some interesting side effects when they're
system controlled.  Triggers are fired by naming convention
(alphabetical order).  If the user creates two triggers, firing first
and third (force_default_value() being second) they will see two
different values in that field.

It would be interesting to see what other DBs do with ALWAYS and BEFORE
triggers (whether they see the user supplied or generated value).



I think a longer term solution would be to add a type to pg_attrdef and
a bool for ALWAYS. (Tom?)
- default, identity, and generator being the types.

- SERIAL would be converted into an identity (not the other way around).

- psql would not show defaults in the case of an identity.

- pg_dump could be trained to use the identity syntax on the table which
has the added benefit of not relying on sequence naming convention.

- rewriteTargetList() can be used to enforce the default value from the
client perspective only (triggers, user rules, etc. could all override
ALWAYS default but the spec does not mention otherwise).

Rod Taylor <rbt@rbt.ca> writes:
> I think a longer term solution would be to add a type to pg_attrdef and
> a bool for ALWAYS. (Tom?)

I thought about it, but won't that change the on-disk format?  Since
minor version upgrades aren't supposed to require a dump / restore,
and I understand 7.4 is already in feature freeze, the earliest
opportunity for something like this would be 7.5.

DES
--
Dag-Erling Smørgrav - des@des.no

On Sun, 2003-08-03 at 10:31, Dag-Erling Smørgrav wrote:
> Rod Taylor <rbt@rbt.ca> writes:
> > I think a longer term solution would be to add a type to pg_attrdef and
> > a bool for ALWAYS. (Tom?)
>
> I thought about it, but won't that change the on-disk format?  Since
> minor version upgrades aren't supposed to require a dump / restore,
> and I understand 7.4 is already in feature freeze, the earliest
> opportunity for something like this would be 7.5.

Yes it would. The solution you have already requires an initdb (changed
pg_proc.h), as such will probably need to wait until 7.5 for
integration. You might be able to squeeze it in as a contrib module for
7.4 though.

des@des.no (Dag-Erling Smørgrav) writes:
> I thought about it, but won't that change the on-disk format?  Since
> minor version upgrades aren't supposed to require a dump / restore,
> and I understand 7.4 is already in feature freeze, the earliest
> opportunity for something like this would be 7.5.

The earliest opportunity is 7.5 anyway.  We don't do feature additions
in dot-releases, only bug fixes.

I don't much care for a trigger-based implementation of the
functionality; it seems like a crude way to do it.  Seems like an easier
answer would involve rejecting attempts to set the column explicitly
during the rewriter stage that inserts default values.

I haven't looked at the SQL200x spec so I'm not sure exactly how they
define GENERATED ALWAYS --- is it an error to try to override the
default, or is your attempt silently ignored?  We could do either.
        regards, tom lane

Rod Taylor <rbt@rbt.ca> writes:
> Yes it would. The solution you have already requires an initdb (changed
> pg_proc.h), as such will probably need to wait until 7.5 for
> integration. You might be able to squeeze it in as a contrib module for
> 7.4 though.

Given that it also requires grammar and parser changes, I don't see how
it could exist as contrib at all.

I recommend dropping any thought of this appearing in any form in any
7.4.* release; it just ain't gonna happen that way.  Design a clean
solution for 7.5, instead.
        regards, tom lane

On Sun, 2003-08-03 at 14:35, Tom Lane wrote:
> Rod Taylor <rbt@rbt.ca> writes:
> > Yes it would. The solution you have already requires an initdb (changed
> > pg_proc.h), as such will probably need to wait until 7.5 for
> > integration. You might be able to squeeze it in as a contrib module for
> > 7.4 though.
>
> Given that it also requires grammar and parser changes, I don't see how
> it could exist as contrib at all.
>
> I recommend dropping any thought of this appearing in any form in any
> 7.4.* release; it just ain't gonna happen that way.  Design a clean
> solution for 7.5, instead.

I see.. I was thinking of the userlocks module which has a partial
implementation in the backend, with the 'work' units in contrib.