Thread: pgsql: Improve handling of inherited GENERATED expressions.

Improve handling of inherited GENERATED expressions.

In both partitioning and traditional inheritance, require child
columns to be GENERATED if and only if their parent(s) are.
Formerly we allowed the case of an inherited column being
GENERATED when its parent isn't, but that results in inconsistent
behavior: the column can be directly updated through an UPDATE
on the parent table, leading to it containing a user-supplied
value that might not match the generation expression.  This also
fixes an oversight that we enforced partition-key-columns-can't-
be-GENERATED against parent tables, but not against child tables
that were dynamically attached to them.

Also, remove the restriction that the child's generation expression
be equivalent to the parent's.  In the wake of commit 3f7836ff6,
there doesn't seem to be any reason that we need that restriction,
since generation expressions are always computed per-table anyway.
By removing this, we can also allow a child to merge multiple
inheritance parents with inconsistent generation expressions, by
overriding them with its own expression, much as we've long allowed
for DEFAULT expressions.

Since we're rejecting a case that we used to accept, this doesn't
seem like a back-patchable change.  Given the lack of field
complaints about the inconsistent behavior, it's likely that no
one is doing this anyway, but we won't change it in minor releases.

Amit Langote and Tom Lane

Discussion: https://postgr.es/m/2793383.1672944799@sss.pgh.pa.us

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/8bf6ec3ba3a44448817af47a080587f3b71bee08

Modified Files
--------------
doc/src/sgml/ddl.sgml                   |  47 ++++++++---
src/backend/commands/tablecmds.c        | 124 +++++++++++-----------------
src/backend/parser/parse_utilcmd.c      |   5 --
src/bin/pg_dump/common.c                |  43 ++++++----
src/bin/pg_dump/pg_dump.c               |  12 +--
src/test/regress/expected/generated.out | 141 ++++++++++++++++++++------------
src/test/regress/sql/generated.sql      |  64 ++++++++-------
7 files changed, 240 insertions(+), 196 deletions(-)

On 2023-01-11 We 15:55, Tom Lane wrote:
> Improve handling of inherited GENERATED expressions.
>
> In both partitioning and traditional inheritance, require child
> columns to be GENERATED if and only if their parent(s) are.
> Formerly we allowed the case of an inherited column being
> GENERATED when its parent isn't, but that results in inconsistent
> behavior: the column can be directly updated through an UPDATE
> on the parent table, leading to it containing a user-supplied
> value that might not match the generation expression.  This also
> fixes an oversight that we enforced partition-key-columns-can't-
> be-GENERATED against parent tables, but not against child tables
> that were dynamically attached to them.
>
> Also, remove the restriction that the child's generation expression
> be equivalent to the parent's.  In the wake of commit 3f7836ff6,
> there doesn't seem to be any reason that we need that restriction,
> since generation expressions are always computed per-table anyway.
> By removing this, we can also allow a child to merge multiple
> inheritance parents with inconsistent generation expressions, by
> overriding them with its own expression, much as we've long allowed
> for DEFAULT expressions.
>
> Since we're rejecting a case that we used to accept, this doesn't
> seem like a back-patchable change.  Given the lack of field
> complaints about the inconsistent behavior, it's likely that no
> one is doing this anyway, but we won't change it in minor releases.


This appears to have caused a problem with cross-version upgrade. It's
not showing on the uploaded buildfarm logs (I will fix that) but the
error from crake is this:

pg_restore: while PROCESSING TOC:

pg_restore: from TOC entry 424; 1259 25257 TABLE gtest_normal_child buildfarm

pg_restore: error: could not execute query: ERROR:  column "b" in child table must not be a generated column

Command was: 

-- For binary upgrade, must preserve pg_type oid

SELECT pg_catalog.binary_upgrade_set_next_pg_type_oid('25259'::pg_catalog.oid);

-- For binary upgrade, must preserve pg_type array oid

SELECT pg_catalog.binary_upgrade_set_next_array_pg_type_oid('25258'::pg_catalog.oid);

-- For binary upgrade, must preserve pg_class oids and relfilenodes

SELECT pg_catalog.binary_upgrade_set_next_heap_pg_class_oid('25257'::pg_catalog.oid);

SELECT pg_catalog.binary_upgrade_set_next_heap_relfilenode('25257'::pg_catalog.oid);

CREATE TABLE "public"."gtest_normal_child" (

    "a" integer,

    "b" integer GENERATED ALWAYS AS (("a" * 2)) STORED

);

-- For binary upgrade, set up inheritance this way.

ALTER TABLE ONLY "public"."gtest_normal_child" INHERIT "public"."gtest_normal";

-- For binary upgrade, set heap's relfrozenxid and relminmxid

UPDATE pg_catalog.pg_class

SET relfrozenxid = '6388', relminmxid = '1'

WHERE oid = '"public"."gtest_normal_child"'::pg_catalog.regclass;



cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, Jan 12, 2023 at 7:52 PM Andrew Dunstan <andrew@dunslane.net> wrote:
> On 2023-01-11 We 15:55, Tom Lane wrote:
> > Improve handling of inherited GENERATED expressions.
> >
> > In both partitioning and traditional inheritance, require child
> > columns to be GENERATED if and only if their parent(s) are.
> > Formerly we allowed the case of an inherited column being
> > GENERATED when its parent isn't, but that results in inconsistent
> > behavior: the column can be directly updated through an UPDATE
> > on the parent table, leading to it containing a user-supplied
> > value that might not match the generation expression.  This also
> > fixes an oversight that we enforced partition-key-columns-can't-
> > be-GENERATED against parent tables, but not against child tables
> > that were dynamically attached to them.
> >
> > Also, remove the restriction that the child's generation expression
> > be equivalent to the parent's.  In the wake of commit 3f7836ff6,
> > there doesn't seem to be any reason that we need that restriction,
> > since generation expressions are always computed per-table anyway.
> > By removing this, we can also allow a child to merge multiple
> > inheritance parents with inconsistent generation expressions, by
> > overriding them with its own expression, much as we've long allowed
> > for DEFAULT expressions.
> >
> > Since we're rejecting a case that we used to accept, this doesn't
> > seem like a back-patchable change.  Given the lack of field
> > complaints about the inconsistent behavior, it's likely that no
> > one is doing this anyway, but we won't change it in minor releases.
>
>
> This appears to have caused a problem with cross-version upgrade. It's
> not showing on the uploaded buildfarm logs (I will fix that) but the
> error from crake is this:
>
> pg_restore: while PROCESSING TOC:
>
> pg_restore: from TOC entry 424; 1259 25257 TABLE gtest_normal_child buildfarm
>
> pg_restore: error: could not execute query: ERROR:  column "b" in child table must not be a generated column
>
> Command was:
>
> -- For binary upgrade, must preserve pg_type oid
>
> SELECT pg_catalog.binary_upgrade_set_next_pg_type_oid('25259'::pg_catalog.oid);
>
> -- For binary upgrade, must preserve pg_type array oid
>
> SELECT pg_catalog.binary_upgrade_set_next_array_pg_type_oid('25258'::pg_catalog.oid);
>
> -- For binary upgrade, must preserve pg_class oids and relfilenodes
>
> SELECT pg_catalog.binary_upgrade_set_next_heap_pg_class_oid('25257'::pg_catalog.oid);
>
> SELECT pg_catalog.binary_upgrade_set_next_heap_relfilenode('25257'::pg_catalog.oid);
>
> CREATE TABLE "public"."gtest_normal_child" (
>
>     "a" integer,
>
>     "b" integer GENERATED ALWAYS AS (("a" * 2)) STORED
>
> );
>
> -- For binary upgrade, set up inheritance this way.
>
> ALTER TABLE ONLY "public"."gtest_normal_child" INHERIT "public"."gtest_normal";

IIUC, by deciding to not back-patch this, we're expecting the user to
fix any child tables in the old cluster that have unsupported (in v16)
properties before upgrading to v16, right?  If that is indeed the
case, maybe this is again a case where TestUpgradeXversion.pm needs
tweaking?

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On 2023-01-12 Th 08:30, Amit Langote wrote:
> On Thu, Jan 12, 2023 at 7:52 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>> On 2023-01-11 We 15:55, Tom Lane wrote:
>>> Improve handling of inherited GENERATED expressions.
>>>
>>> In both partitioning and traditional inheritance, require child
>>> columns to be GENERATED if and only if their parent(s) are.
>>> Formerly we allowed the case of an inherited column being
>>> GENERATED when its parent isn't, but that results in inconsistent
>>> behavior: the column can be directly updated through an UPDATE
>>> on the parent table, leading to it containing a user-supplied
>>> value that might not match the generation expression.  This also
>>> fixes an oversight that we enforced partition-key-columns-can't-
>>> be-GENERATED against parent tables, but not against child tables
>>> that were dynamically attached to them.
>>>
>>> Also, remove the restriction that the child's generation expression
>>> be equivalent to the parent's.  In the wake of commit 3f7836ff6,
>>> there doesn't seem to be any reason that we need that restriction,
>>> since generation expressions are always computed per-table anyway.
>>> By removing this, we can also allow a child to merge multiple
>>> inheritance parents with inconsistent generation expressions, by
>>> overriding them with its own expression, much as we've long allowed
>>> for DEFAULT expressions.
>>>
>>> Since we're rejecting a case that we used to accept, this doesn't
>>> seem like a back-patchable change.  Given the lack of field
>>> complaints about the inconsistent behavior, it's likely that no
>>> one is doing this anyway, but we won't change it in minor releases.
>>
>> This appears to have caused a problem with cross-version upgrade. It's
>> not showing on the uploaded buildfarm logs (I will fix that) but the
>> error from crake is this:
>>
>> pg_restore: while PROCESSING TOC:
>>
>> pg_restore: from TOC entry 424; 1259 25257 TABLE gtest_normal_child buildfarm
>>
>> pg_restore: error: could not execute query: ERROR:  column "b" in child table must not be a generated column
>>
>> Command was:
>>
>> -- For binary upgrade, must preserve pg_type oid
>>
>> SELECT pg_catalog.binary_upgrade_set_next_pg_type_oid('25259'::pg_catalog.oid);
>>
>> -- For binary upgrade, must preserve pg_type array oid
>>
>> SELECT pg_catalog.binary_upgrade_set_next_array_pg_type_oid('25258'::pg_catalog.oid);
>>
>> -- For binary upgrade, must preserve pg_class oids and relfilenodes
>>
>> SELECT pg_catalog.binary_upgrade_set_next_heap_pg_class_oid('25257'::pg_catalog.oid);
>>
>> SELECT pg_catalog.binary_upgrade_set_next_heap_relfilenode('25257'::pg_catalog.oid);
>>
>> CREATE TABLE "public"."gtest_normal_child" (
>>
>>     "a" integer,
>>
>>     "b" integer GENERATED ALWAYS AS (("a" * 2)) STORED
>>
>> );
>>
>> -- For binary upgrade, set up inheritance this way.
>>
>> ALTER TABLE ONLY "public"."gtest_normal_child" INHERIT "public"."gtest_normal";
> IIUC, by deciding to not back-patch this, we're expecting the user to
> fix any child tables in the old cluster that have unsupported (in v16)
> properties before upgrading to v16, right?  If that is indeed the
> case, maybe this is again a case where TestUpgradeXversion.pm needs
> tweaking?
>

OK, please tell me exactly what I'm meant to tweak :-)


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-12 Th 08:30, Amit Langote wrote:
>> IIUC, by deciding to not back-patch this, we're expecting the user to
>> fix any child tables in the old cluster that have unsupported (in v16)
>> properties before upgrading to v16, right?  If that is indeed the
>> case, maybe this is again a case where TestUpgradeXversion.pm needs
>> tweaking?

> OK, please tell me exactly what I'm meant to tweak :-)

Yeah.  I think best bet may be to drop the problematic table(s),
as we've done for other de-supported DDL situations.  A quick
experiment suggests that

DROP TABLE gtest_normal_child;
DROP TABLE gtest_normal_child2;

should do it.

I can work on a better-tested patch for TestUpgradeXversion.pm, but
I have a couple other commitments today so it may take a bit of time.

            regards, tom lane

I wrote:
> Yeah.  I think best bet may be to drop the problematic table(s),
> as we've done for other de-supported DDL situations.  A quick
> experiment suggests that
> DROP TABLE gtest_normal_child;
> DROP TABLE gtest_normal_child2;
> should do it.

Some testing here confirms that the attached patch is enough to
un-break things for HEAD.  Since you mentioned you have some other
quasi-urgent BF fixes, maybe the thing to do is to roll up a release
now and then work on the bigger idea at leisure.

            regards, tom lane

diff -pudr client-code-REL_15.orig/PGBuild/Modules/TestUpgradeXversion.pm
client-code-REL_15/PGBuild/Modules/TestUpgradeXversion.pm
--- client-code-REL_15.orig/PGBuild/Modules/TestUpgradeXversion.pm    2022-12-31 09:15:03.000000000 -0500
+++ client-code-REL_15/PGBuild/Modules/TestUpgradeXversion.pm    2023-01-12 15:27:30.709009163 -0500
@@ -586,12 +586,17 @@ sub test_upgrade    ## no critic (Subrou
         }
     }

-    # can't upgrade aclitem in user tables from pre 16 to 16+
+    # stuff not supported from release 16
     if (   ($this_branch gt 'REL_15_STABLE' || $this_branch eq 'HEAD')
         && ($oversion le 'REL_15_STABLE' && $oversion ne 'HEAD'))
     {
-        my $prstmt = "alter table if exists public.tab_core_types
-                      drop column if exists aclitem";
+        # Can't upgrade aclitem in user tables from pre 16 to 16+.
+        # Also can't handle child tables with newly-generated columns.
+        my $prstmt = join(';',
+                          'alter table if exists public.tab_core_types
+                          drop column if exists aclitem',
+                          'drop table if exists public.gtest_normal_child',
+                          'drop table if exists public.gtest_normal_child2');

         run_psql("$other_branch/inst/bin/psql", "-e", $prstmt,
             "regression", "$upgrade_loc/$oversion-copy.log", 1);

On 2023-01-12 Th 15:44, Tom Lane wrote:
> I wrote:
>> Yeah.  I think best bet may be to drop the problematic table(s),
>> as we've done for other de-supported DDL situations.  A quick
>> experiment suggests that
>> DROP TABLE gtest_normal_child;
>> DROP TABLE gtest_normal_child2;
>> should do it.
> Some testing here confirms that the attached patch is enough to
> un-break things for HEAD.  Since you mentioned you have some other
> quasi-urgent BF fixes, maybe the thing to do is to roll up a release
> now and then work on the bigger idea at leisure.
>
>             


Thanks, that's almost exactly what I did to turn crake green.

I'll push out a release tomorrow.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com