pgsql: Improve handling of inherited GENERATED expressions.

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

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

On Fri, Jan 13, 2023 at 6:41 Andrew Dunstan <andrew@dunslane.net> wrote:

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.

Thanks to both of you.

--

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