Re: [PATCH v1] Fix for bug #19773 : pg_dump differing generation expressions of inherited columns

Hi Nitin,

> I'm attaching a patch with a proposed fix. It changes getTableAttrs()
> which previously used to always mark 'separate = false' for generation
> expressions. That branch has been removed and now those expressions
> are handled like any other column. It also changes dumpAttrDef() to
> correctly dump the ALTER command with the generation expression.

Thanks for the patch. I think emitting a separate ALTER TABLE ONLY
... SET EXPRESSION for the child is the right direction, since it
keeps the column non-local (attislocal = false). Both reported cases
restore with the right expressions and values, for STORED and
VIRTUAL.

I had been tracking the same defect myself, with a somewhat
different fix, so I checked whether the problems I ran into there
also occur with v1 on master (d6cf162f74a). They do, in the cases
below. I have not found a solution for them yet either, so I am only
sharing what I see.

> It also adds an ordering constraint in dumpAttrDef() to handle
> multi-level hierarchy and ALTERs in generation expressions at multiple
> levels.

1. pg_dump does not finish

create table c (x int, z int);
create function h() returns int language sql stable
begin atomic select count(*)::int from c; end;
create table a (x int, z int default h(),
g int generated always as (x * 2) stored);
create table b () inherits (a);
alter table c add column g int generated always as (x * 2) stored;
alter table c inherit b;

Unpatched pg_dump finishes normally. The patched pg_dump keeps
spinning in sortDumpableObjects() (killed by a 20 second alarm,
rc=142). The loop is ("->" meaning "must come after"):

TABLE c -> ATTRDEF b.g new edge: c.g has b.g's expression
ATTRDEF b.g -> TABLE b
TABLE b -> TABLE a INHERITS
TABLE a -> ATTRDEF a.z default h() printed inline
ATTRDEF a.z -> FUNCTION h
FUNCTION h -> TABLE c the body reads c

and the new "child table -> parent attrdef" edge closes it. The
"Indirect loop" repair picks the pair (b, b.g). b.g is already
separate and there is no b -> b.g edge to remove, so it only adds
another b.g -> b edge and the same loop is found again. b.g is not
even dumped here. I could reproduce the same hang when b.g is
dumped (its expression changed on the middle table).

2. Multi-level cases where the child still gets the parent's old
expression

(a) When the loop cannot be resolved:

CREATE TABLE a (x int, g int GENERATED ALWAYS AS (x * 2) STORED);
CREATE TABLE b () INHERITS (a);
CREATE TABLE c () INHERITS (b);
CREATE FUNCTION f3(int) RETURNS int IMMUTABLE LANGUAGE sql
BEGIN ATOMIC
SELECT $1 * 3 + (SELECT count(*)::int FROM public.c WHERE false);
END;
ALTER TABLE b ALTER COLUMN g SET EXPRESSION AS (f3(x));

pg_dump warns "could not resolve dependency loop" (TABLE c, ATTRDEF
b.g, FUNCTION f3) and exits with rc=0. CREATE TABLE c comes before
b's SET EXPRESSION, so after restore c.g is (x * 2) instead of
f3(x). Before the patch the restore failed at b's SET DEFAULT
('column "g" of relation "b" is a generated column'), leaving b.g
and c.g as (x * 2). Now b.g is right, but c.g stays (x * 2) with no
restore error; only pg_dump's loop warning hints at it.

(b) When the parent's column is local:

create table g (a int, g int generated always as (a*2) stored);
create table p (a int, g int);
create function f(x int) returns int immutable language sql
begin atomic select x * 3 + 0 * (select count(*) from p)::int; end;
alter table p drop column g;
alter table p add column g int generated always as (f(a)) stored;
alter table p inherit g;
create table c () inherits (p);
insert into c values (1);

p.g is local, so it is not separate when flagInhAttrs() runs and no
edge is added. The loop repair during sorting makes it separate
later and splits out the SET EXPRESSION. The dump puts CREATE TABLE
c before ALTER TABLE ONLY public.p ... SET EXPRESSION, and the
restored c.g is (a * 2) with the row (1,2), where the source has
f(a) and (1,3). The restore reports no error. Without the patch
SET DEFAULT failed there, leaving both p.g and c.g as (a * 2).

3. The restored child loses its inherited CHECK constraint

CREATE TABLE p (x int, g int GENERATED ALWAYS AS (x * 2) STORED);
CREATE TABLE c () INHERITS (p);
ALTER TABLE ONLY c ALTER COLUMN g SET EXPRESSION AS (x * 9);
ALTER TABLE p ADD CONSTRAINT p_g_check CHECK (g >= 0);

The dump has, after CREATE TABLE public.c:

ALTER TABLE ONLY public.c ALTER COLUMN g SET EXPRESSION AS ((x * 9));

In src and in dst (dumped and restored with the patched pg_dump):

SELECT conrelid::regclass, conname, conislocal, coninhcount
FROM pg_constraint WHERE conname = 'p_g_check' ORDER BY 1;
INSERT INTO c VALUES (-1);

== src
conrelid | conname | conislocal | coninhcount
----------+-----------+------------+-------------
p | p_g_check | t | 0
c | p_g_check | f | 1
(2 rows)

ERROR: new row for relation "c" violates check constraint "p_g_check"
DETAIL: Failing row contains (-1, -9).

== dst
conrelid | conname | conislocal | coninhcount
----------+-----------+------------+-------------
p | p_g_check | t | 0
(1 row)

INSERT 0 1

The restore reports no error. The cause seems to be in the backend:
on master without the patch, running ALTER TABLE ONLY c ... SET
EXPRESSION while the inherited CHECK already exists also drops c's
copy (ATPostAlterTypeCleanup recreates only conislocal constraints).
Inherited NOT NULL constraints lose c's pg_constraint row the same
way (attnotnull stays set, so NULLs are still rejected), and a
merged local+inherited CHECK ends up with coninhcount 0 instead of
1. The patch emits that command whenever a child's expression
differs from its parent's, so the problem now shows up in a plain
dump/restore. A later pg_upgrade of the restored
cluster passes --check and then fails with "child table is missing
constraint".

Since this happens on master without the patch, I think it is
better handled as a separate backend bug, apart from this patch.

Best regards,
Henson