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

On Thu, Jan 25, 2018 at 10:26:55PM -0500, Peter Eisentraut wrote:
> On 1/19/18 00:18, Michael Paquier wrote:
>> +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.

I have been looking at that case more closely again, and on the contrary
I would advocate that your patch is doing the *right* thing.  In short,
if the generation expression uses a function and the user has only been
granted access to read the values, it seems to me that it we should
require that this user also has the right to execute the function. Would
that be too user-unfriendly?  I think that this could avoid mistakes
about giving access to unwanted functions when willing to just give a
SELECT right as the function could be doing more operations.

>> +-- 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.

Okay, let's leave with this restriction for now.

>> 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.)

Hm.  Identity columns and default columns are part of rowtypes. STORED
columns can alsobe part of a row type with little effort, so in order to
be consistent with the all the existing behaviors, it seems to me that
virtually-generated columns should be part of it as well.  I have
compiled in the attached SQL file how things behave with your
patch.  Only virtually-generated columns show a blank value.

A empty value is unhelpful for the user, which brings a couple of
possible approaches:
1) Make virtual columns part of a row type, which would make it
consistent with all the other types.
2) For plpgsql, if all rows from OLD or NEW are requested, then print all
columns except the ones virtually-generated. If a virtual column is
directly requested, then issue an error.

I would warmly welcome 1) as this would make all behaviors consistent
with the other PLs and other types of generated columns.  I would
imagine that users would find weird the current inconsistency as well.

>> 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.

tableoid can be deterministic if compared with data from pg_class:
=# CREATE TABLE aa (a int PRIMARY KEY, b int GENERATED ALWAYS AS (tableoid));
CREATE TABLE
=# INSERT INTO aa VALUES (1);
INSERT
=# SELECT aa.a from pg_class c, aa where c.oid = b;
 a
---
 1
(1 row)

The point is just to stress code paths where the attribute number is
negative.

>> -    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.

Looks like I messed up ATPrepAlterColumnType() and ATExecColumnDefault()
while reading the previous version.  Sorry for the useless noise.

+    /*
+     * Foreign keys on generated columns are not yet implemented.
+     */
+    for (i = 0; i < numpks; i++)
+    {
+        if (get_attgenerated(RelationGetRelid(pkrel), pkattnum[i]))
+            ereport(ERROR,
+                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                     errmsg("foreign key constraints referencing generated columns are not supported")));
+    }
It would be nice to test this code path.

The commit fest is ending, perhaps this should be moved to the next one?
The handling of row types for virtual columns is a must-fix in my
opinion.
--
Michael