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

Hi Nitin,

I have looked at v2. Below are my notes on the multi-parent scenario,
and on how the backend problem where the restored child loses its
inherited CHECK constraint shows up with v2.

> Note this still fails for the following multi-parent scenario.
> [...]
> This scenario also fails on master and defaults seem to have the
> same limitation there. So I haven't tried to fix with any other
> special case. Let me know what you think of this.

I checked this scenario. The restore fails at CREATE TABLE c with:

ERROR: column "gs" inherits conflicting generation expressions

Master fails at the same statement with the same error, so I don't
think this failure takes anything away from the improvement. v2 fixes
the single-parent multi-level cases that silently restored wrong
expressions and values. The multi-parent case is not a new defect
introduced by this patch but a separate one that was already there, so
I agree with leaving it out of scope for this patch.

> > Since this happens on master without the patch, I think it is
> > better handled as a separate backend bug, apart from this patch.
>
> Regarding this item, are you planning to work on this? If not, I am
> happy to take it up.

Thanks again for taking this up. Meanwhile, I checked how this
problem shows up with v2. After a dump and restore with v2, the
expressions come back right, but inherited CHECK constraints are lost:

CREATE TABLE a (x int, g int GENERATED ALWAYS AS (x * 2) STORED);
CREATE TABLE b () INHERITS (a);
CREATE TABLE c () INHERITS (b);
ALTER TABLE b ALTER COLUMN g SET EXPRESSION AS (x * 3);
ALTER TABLE a ADD CONSTRAINT a_chk CHECK (g < 1000);
INSERT INTO c VALUES (1);

v2 emits one command each for b and c:

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

The restore finishes without errors, but

SELECT conrelid::regclass AS rel, conislocal, coninhcount
FROM pg_constraint WHERE conname = 'a_chk' ORDER BY 1;

gives, in the original and in the restored database:

original restored
rel | conislocal | coninhcount rel | conislocal | coninhcount
-----+------------+------------- -----+------------+-------------
a | t | 0 a | t | 0
b | f | 1
c | f | 1

So in the restored database INSERT INTO c VALUES (500) succeeds and
stores a row with g = 1500, while in the original it fails with "new
row for relation "c" violates check constraint "a_chk"".

Here are the original and the two restores, in execution order.

Original (the script above, without the INSERT):

CREATE TABLE a (x int, g int GENERATED ALWAYS AS (x * 2) STORED);
CREATE TABLE b () INHERITS (a);
CREATE TABLE c () INHERITS (b);
ALTER TABLE b ALTER COLUMN g SET EXPRESSION AS (x * 3);
ALTER TABLE a ADD CONSTRAINT a_chk CHECK (g < 1000);
-- a_chk: a, b, c

Restore (dump without the patch):

CREATE TABLE a (x int, g int GENERATED ALWAYS AS (x * 2) STORED,
CONSTRAINT a_chk CHECK (g < 1000));
CREATE TABLE b () INHERITS (a);
CREATE TABLE c () INHERITS (b);
-- a_chk: a, b, c
-- b.g, c.g: (x * 2)

Restore (v2 dump):

CREATE TABLE a (x int, g int GENERATED ALWAYS AS (x * 2) STORED,
CONSTRAINT a_chk CHECK (g < 1000));
CREATE TABLE b () INHERITS (a);
CREATE TABLE c () INHERITS (b);
ALTER TABLE ONLY b ALTER COLUMN g SET EXPRESSION AS (x * 3);
ALTER TABLE ONLY c ALTER COLUMN g SET EXPRESSION AS (x * 3);
-- a_chk: a

What decides the result is which of the CHECK and the SET EXPRESSION
comes first; ONLY and an inline CHECK make no difference. It also
shows without pg_dump. With a row (1) in b and the expression changed
to (x * 3000):

- original order: ADD CONSTRAINT fails with "check constraint
"a_chk" of relation "b" is violated by some row".
- restore order: ONLY b ... SET EXPRESSION succeeds, b keeps
g = 3000, and b's a_chk is gone.
- a_chk as b's own local CHECK: SET EXPRESSION fails with the same
error.

SET EXPRESSION drops the constraints that depend on the column and
recreates only the local ones (ATPostAlterTypeCleanup()). Inherited
ones are expected to come back when the parent's constraint is
re-added, which does not happen when only the child is altered.

Inherited NOT NULL constraints are lost the same way, as I wrote in my
earlier review. With ALTER TABLE a ALTER COLUMN g SET NOT NULL in
place of the CHECK in the script above, the database restored with v2
has no NOT NULL constraint row for b and c (attnotnull stays set), and
a later DROP NOT NULL on the parent fails with "cache lookup failed
for not-null constraint". So the backend fix needs to cover NOT NULL
as well.

That is, the CHECK loss is not a defect of this patch but a backend
defect of SET EXPRESSION. The CHECK and the SET EXPRESSION should
give the same result in either order, and they do not, and it happens
without pg_dump as well. A dump without the patch does not hit it
only because it never emits SET EXPRESSION.

So I see both the multi-parent case and the loss of inherited CHECK
and NOT NULL constraints as pre-existing defects separate from this
patch. I will look at the backend fix once it is posted.

One small comment on the code: the new field
maybe_dump_later_if_altered_expression is much longer than its
neighbours in AttrDefInfo (adtable, adnum, adef_expr, separate), and
it names what may happen later rather than the state it records. A
shorter name such as inherited_expr, with the existing comment
explaining it, would read better next to separate.

Regards,
Henson