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

On 1/19/18 00:18, Michael Paquier wrote:
> Instead of leaving bits for a feature that may or may not be
> implemented, have you considered just blocking STORED at parsing level
> and remove those bits? There is little point in keeping the equivalent
> of dead code in the tree. So I would suggest a patch simplification:
> - Drop the VIRTUAL/STORED parsing from the grammar for now.
> - Define VIRTUAL as the default for the future.

fixed

> =# CREATE TABLE gen_1 (a int, b int GENERATED ALWAYS AS (a * 2)
> VIRTUAL);
> CREATE TABLE
> =# insert into gen_1 values (2000000000);
> INSERT 0 1
> =# select * from gen_1 ;
> ERROR:  22003: integer out of range
> Overflow checks do not happen when inserting, which makes the following
> SELECT to fail. This could be confusing for the user because the row has
> been inserted. Perhaps some evaluation of virtual tuples at insert phase
> should happen. The existing behavior makes sense as well as virtual
> values are only evaluated when read, so a test would be at least
> welcome.

added test

> Does the SQL spec mention the matter? How do other systems
> handle such cases?

In Oracle you get the same overflow error.

> CHECK constraints can be combined, still..

I think you can compare this to a view.  A view can produce overflow
errors on read.  But a CHECK constraint is more like a CHECK option on a
view that checks as values are put into the view.

> The last patch crashes for typed tables:
> =# CREATE TYPE itest_type AS (f1 integer, f2 text, f3 bigint);
> CREATE TYPE
> =# CREATE TABLE itest12 OF itest_type (f1 WITH OPTIONS GENERATED ALWAYS
> AS (f2 *2));
> [... boom ...]
> And for partitions:
> =# CREATE TABLE itest_parent (f1 date NOT NULL, f2 text, f3 bigint)
>    PARTITION BY RANGE (f1);
> CREATE TABLE
> =# CREATE TABLE itest_child PARTITION OF itest_parent (f3 WITH OPTIONS
>    GENERATED ALWAYS AS (f3)) FOR VALUES FROM ('2016-07-01') TO
>    ('2016-08-01');
> [... boom ...]
> Like what we did in 005ac298, I would suggest to throw
> ERRCODE_FEATURE_NOT_SUPPORTED. Please also add some tests.

done

> +SELECT a, c FROM gtest12;  -- FIXME: should be allowed
> +ERROR:  permission denied for function gf1

This is quite hard to fix and I would like to leave this for a future
release.

> +ALTER TABLE gtest27 ALTER COLUMN b DROP DEFAULT;  -- FIXME
> +ERROR:  column "b" of relation "gtest27" is a generated column

That FIXME seems to have been a mistake.  I have removed it.

> +    if (get_attgenerated(relationId, attno))
> +        ereport(ERROR,
> +            (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +             errmsg("index creation on generated columns is not supported")));
> Shouldn't such messages mention explicitely "virtually"-generated
> columns? For stored columns the support would not be complicated in this
> case.

done

> +-- domains
> +CREATE DOMAIN gtestdomain1 AS int CHECK (VALUE < 10);
> +CREATE TABLE gtest24 (a int PRIMARY KEY, b gtestdomain1 GENERATED
> ALWAYS AS (a * 2));  -- prohibited
> +ERROR:  virtual generated column "b" cannot have a domain type
> CHECK constraints can be used, so I find this restriction confusing.

We currently don't have infrastructure to support this.  We run all
CHECK constraints whenever a row is changed, so that is easy.  But we
don't have facilities to recheck the domain constraint in column b when
column a is updated.  This could be done, but it's extra work.

> No test coverage for DELETE triggers?

done

> +UPDATE gtest26 SET a = a * -2;
> +INFO:  gtest1: old = (-2,)
> +INFO:  gtest1: new = (4,)
> +INFO:  gtest3: old = (-2,)
> +INFO:  gtest3: new = (4,)
> +INFO:  gtest4: old = (3,)
> +INFO:  gtest4: new = (-6,)
> OLD and NEW values for generated columns don't show up. Am I missing
> something or they should be available?

This was already discussed a few times in the thread.  I don't know what
a good solution is.

I have in this patch added facilties to handle this better in other PLs.
 So the virtual generated column doesn't show up there in the trigger
data.  This is possible because we copy the trigger data from the
internal structures into language-specific hashes/dictionaries/etc.

In PL/pgSQL, this is a bit more difficult, because we handle the table's
row type there.  We can't just "hide" the generated column when looking
at the row type.  Actually, we could do it quite easily, but that would
probably raise other weirdnesses.

This also raises a question how a row type with generated columns should
behave.  I think a generation expression is a property of a table, so it
does not apply in a row type.  (Just like a default is a property of a
table and does not apply in row types.)

> Please use brackers here if you use a comment in the if() block...
> [/nit_mode]

done

> COPY as you are proposing looks sensible to me. I am not sure about any
> options though as it is possible to enforce the selection of generated
> columns using COPY (SELECT).

removed that comment

> Per my tests, generated columns can work with column system attributes
> (xmax, xmin, etc.). Some tests could be added.

Hard to test that, because the results would be nondeterministic.

> -    if (tab->relkind == RELKIND_RELATION ||
> -        tab->relkind == RELKIND_PARTITIONED_TABLE)
> +    if ((tab->relkind == RELKIND_RELATION ||
> +         tab->relkind == RELKIND_PARTITIONED_TABLE) &&
> +        get_attgenerated(RelationGetRelid(rel), attnum) != ATTRIBUTE_GENERATE
> I think that you should store the result of get_attgenerated() and reuse
> it multiple times.

I don't see where that would apply.  I think the hunks you are seeing
belong to different functions.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services