Thread: Dumping/restoring fails on inherited generated column

Run the regression tests with "make installcheck", then:

$ pg_dump -Fc regression >r.dump
$ createdb r2
$ pg_restore -d r2 r.dump
pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 6005; 2604 24821 DEFAULT gtest1_1 b postgres
pg_restore: error: could not execute query: ERROR:  column "b" of relation "gtest1_1" is a generated column
Command was: ALTER TABLE ONLY public.gtest1_1 ALTER COLUMN b SET DEFAULT (a * 2);


pg_restore: warning: errors ignored on restore: 1
$

It looks like gtest1_1 inherits column "b" from gtest1, so possibly
pg_dump is just confused about the combination of inheritance and
generated columns.

I see this in v12 as well as HEAD.  One interesting question is how come
the pg_upgrade test isn't failing --- maybe binary-upgrade mode handles
this case differently?

            regards, tom lane

On 2019-12-04 15:14, Tom Lane wrote:
> Run the regression tests with "make installcheck", then:
> 
> $ pg_dump -Fc regression >r.dump
> $ createdb r2
> $ pg_restore -d r2 r.dump
> pg_restore: while PROCESSING TOC:
> pg_restore: from TOC entry 6005; 2604 24821 DEFAULT gtest1_1 b postgres
> pg_restore: error: could not execute query: ERROR:  column "b" of relation "gtest1_1" is a generated column
> Command was: ALTER TABLE ONLY public.gtest1_1 ALTER COLUMN b SET DEFAULT (a * 2);
> 
> 
> pg_restore: warning: errors ignored on restore: 1
> $
> 
> It looks like gtest1_1 inherits column "b" from gtest1, so possibly
> pg_dump is just confused about the combination of inheritance and
> generated columns.

Yeah, there was some stuff about the "separate" dumping of defaults that 
I apparently forgot to address.  The attached patch fixes it.  I'll see 
about adding a test case for it, too.

> I see this in v12 as well as HEAD.  One interesting question is how come
> the pg_upgrade test isn't failing --- maybe binary-upgrade mode handles
> this case differently?

Binary upgrade dumps out even inherited columns, so it won't run into 
the "separate" case that's the issue here.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-12-04 15:14, Tom Lane wrote:
>> It looks like gtest1_1 inherits column "b" from gtest1, so possibly
>> pg_dump is just confused about the combination of inheritance and
>> generated columns.

> Yeah, there was some stuff about the "separate" dumping of defaults that 
> I apparently forgot to address.  The attached patch fixes it.  I'll see 
> about adding a test case for it, too.

I don't think this is right.  It will probably misbehave if the
"generated" expression has any interesting dependencies:

1. You didn't duplicate the behavior of the existing separate=false
case, where it adds a dependency to try to force the default's
dependencies to exist before the table is created.

2. If that dependency turns out to create a dependency loop, the
later code will break the loop by setting separate=true anyway.
Then what?

I also find it improbable that overriding the !shouldPrintColumn
test is really the right thing.  That test is what distinguishes
the is-a-parent-table from the is-a-child-table cases, and the
core of the issue here seems to be that we need to treat those
differently.

I wonder if the right fix is to not generate a DO_ATTRDEF
object at all for generated columns in child tables.  Am
I right in guessing that we propagate generated-ness to
child tables automatically?

            regards, tom lane

On 2019-12-04 21:36, Tom Lane wrote:
> I wonder if the right fix is to not generate a DO_ATTRDEF
> object at all for generated columns in child tables.  Am
> I right in guessing that we propagate generated-ness to
> child tables automatically?

Right.  New patch using that approach attached.  (Could use more 
extensive comments.)

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-12-04 21:36, Tom Lane wrote:
>> I wonder if the right fix is to not generate a DO_ATTRDEF
>> object at all for generated columns in child tables.  Am
>> I right in guessing that we propagate generated-ness to
>> child tables automatically?

> Right.  New patch using that approach attached.  (Could use more 
> extensive comments.)

This looks more plausible than the previous attempt, but it's clearly
still not right, because this is what it changes in the regression
test dump:

--- r.dump.head    2020-02-03 14:16:15.774305437 -0500
+++ r.dump.patch    2020-02-03 14:18:08.599109703 -0500
@@ -15244,14 +15244,7 @@
 -- Name: gtest1_1 b; Type: DEFAULT; Schema: public; Owner: postgres
 --
 
-ALTER TABLE ONLY public.gtest1_1 ALTER COLUMN b SET DEFAULT (a * 2);
-
-
---
--- Name: gtest30_1 b; Type: DEFAULT; Schema: public; Owner: postgres
---
-
-ALTER TABLE ONLY public.gtest30_1 ALTER COLUMN b SET DEFAULT (a * 2);
+ALTER TABLE ONLY public.gtest1_1 ALTER COLUMN b SET DEFAULT NULL;
 
 
 --

This is showing us at least two distinct problems.  Now as for
"gtest30_1", what we have is that in the parent table "gtest30", column b
exists but it has no default; the generated property is only added
at the child table gtest30_1.  So we need to emit ALTER COLUMN SET
GENERATED ALWAYS for gtest30_1.b.  HEAD is already doing the wrong
thing there (it's emitting the expression, but as a plain default
not GENERATED).  And this patch makes it emit nothing, even worse.
I think the key point here is that "attislocal" refers to whether the
column itself is locally defined, not to whether its default is.

Things are evidently also going wrong for "gtest1_1".  In that case
the generated property is inherited from the parent gtest1, so we
shouldn't be emitting anything ... how come the patch fails to
make it do that?

            regards, tom lane

On 2020-02-03 20:32, Tom Lane wrote:
 > Things are evidently also going wrong for "gtest1_1".  In that case
 > the generated property is inherited from the parent gtest1, so we
 > shouldn't be emitting anything ... how come the patch fails to
 > make it do that?

This is fixed by the attached new patch.  It needed an additional check 
in flagInhAttrs().

> This is showing us at least two distinct problems.  Now as for
> "gtest30_1", what we have is that in the parent table "gtest30", column b
> exists but it has no default; the generated property is only added
> at the child table gtest30_1.  So we need to emit ALTER COLUMN SET
> GENERATED ALWAYS for gtest30_1.b.  HEAD is already doing the wrong
> thing there (it's emitting the expression, but as a plain default
> not GENERATED).  And this patch makes it emit nothing, even worse.
> I think the key point here is that "attislocal" refers to whether the
> column itself is locally defined, not to whether its default is.

This is a bit of a mess.  Let me explain my thinking on generated 
columns versus inheritance.

If a parent table has a generated column, then any inherited column must 
also be generated and use the same expression.  (Otherwise querying the 
parent table would produce results that are inconsistent with the 
generation expression if the rows come from the child table.)

If a parent table has a column that is not generated, then I think it 
would be semantically sound if a child table had that same column but 
generated.  However, I think it would be very tricky to support this 
correctly, and it doesn't seem useful enough, so I'd rather not do it.

That's what the gtest30_1 case above shows.  It's not even clear whether 
it's possible to dump this correctly in all cases because the syntax 
that you allude to "turn this existing column into a generated column" 
does not exist.

Note that the gtest30 test case is new in master.  I'm a bit confused 
why things were done that way, and I'll need to revisit this.  I've also 
found a few more issues with how certain combinations of DDL can create 
similar situations that arguably don't make sense, and I'll continue to 
look into those.  Basically, my contention is that gtest30_1 should not 
be allowed to exist like that.

However, the pg_dump issue is separate from those because it affects a 
case that is clearly legitimate.  So assuming that we end up agreeing on 
a version of the attached pg_dump patch, I would like to get that into 
the next minor releases and then investigate the other issues separately.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2020-02-03 20:32, Tom Lane wrote:
>> This is showing us at least two distinct problems.  Now as for
>> "gtest30_1", what we have is that in the parent table "gtest30", column b
>> exists but it has no default; the generated property is only added
>> at the child table gtest30_1.  So we need to emit ALTER COLUMN SET
>> GENERATED ALWAYS for gtest30_1.b.  HEAD is already doing the wrong
>> thing there (it's emitting the expression, but as a plain default
>> not GENERATED).  And this patch makes it emit nothing, even worse.
>> I think the key point here is that "attislocal" refers to whether the
>> column itself is locally defined, not to whether its default is.

> This is a bit of a mess.  Let me explain my thinking on generated 
> columns versus inheritance.

> If a parent table has a generated column, then any inherited column must 
> also be generated and use the same expression.  (Otherwise querying the 
> parent table would produce results that are inconsistent with the 
> generation expression if the rows come from the child table.)

Check.

> If a parent table has a column that is not generated, then I think it 
> would be semantically sound if a child table had that same column but 
> generated.  However, I think it would be very tricky to support this 
> correctly, and it doesn't seem useful enough, so I'd rather not do it.

So ... why is that so hard exactly?  AFAICS, the existing regression
test cases show that it works fine.  Except that pg_dump gets it wrong.
In general, we surely want to support child columns that have defaults
different from the parent column's default, so this doesn't seem quite
that huge a leap to me.

> That's what the gtest30_1 case above shows.  It's not even clear whether 
> it's possible to dump this correctly in all cases because the syntax 
> that you allude to "turn this existing column into a generated column" 
> does not exist.

I'm a little confused by that statement.  What is this doing, if not
that:

regression=# create table foo (f1 int not null);
CREATE TABLE
regression=# alter table foo alter column f1 add generated always as identity;
ALTER TABLE
regression=# \d foo
                           Table "public.foo"
 Column |  Type   | Collation | Nullable |           Default            
--------+---------+-----------+----------+------------------------------
 f1     | integer |           | not null | generated always as identity

If we didn't have things like ALTER ... SET GENERATED and
ALTER ... DROP EXPRESSION, I'd be a lot more content to accept
the position that generated-ness is an immutable column property.
But we do have those things, so the restriction you're proposing
seems mighty arbitrary.

            regards, tom lane

I arrived at this thread while investigating the same issue recently
reported[1].

On Fri, 7 Feb 2020 at 04:36, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
>
> On 2020-02-03 20:32, Tom Lane wrote:
>  > Things are evidently also going wrong for "gtest1_1".  In that case
>  > the generated property is inherited from the parent gtest1, so we
>  > shouldn't be emitting anything ... how come the patch fails to
>  > make it do that?
>
> This is fixed by the attached new patch.  It needed an additional check
> in flagInhAttrs().
>
> > This is showing us at least two distinct problems.  Now as for
> > "gtest30_1", what we have is that in the parent table "gtest30", column b
> > exists but it has no default; the generated property is only added
> > at the child table gtest30_1.  So we need to emit ALTER COLUMN SET
> > GENERATED ALWAYS for gtest30_1.b.  HEAD is already doing the wrong
> > thing there (it's emitting the expression, but as a plain default
> > not GENERATED).  And this patch makes it emit nothing, even worse.
> > I think the key point here is that "attislocal" refers to whether the
> > column itself is locally defined, not to whether its default is.
>
> This is a bit of a mess.  Let me explain my thinking on generated
> columns versus inheritance.
>
> If a parent table has a generated column, then any inherited column must
> also be generated and use the same expression.  (Otherwise querying the
> parent table would produce results that are inconsistent with the
> generation expression if the rows come from the child table.)

After investigating this issue, I think that current DDLs regarding
inherited tables and generated columns seem not to work fine.

We can make an inherited table have the same column having a different
generation expression as follows:

=# create table p1 (a int, b int generated always as (a + 1) stored);
=# create table c1 (a int, b int generated always as (a + 2) stored)
inherits(p1);

But the column on the inherited table has a default value, the column
will be generation expression with a const value:

=# create table p2 (a int, b int generated always as (a + 1) stored);
=# create table c2 (a int, b int default 10) inherits(p2);
=# \d c2
                             Table "public.c2"
 Column |  Type   | Collation | Nullable |             Default
--------+---------+-----------+----------+---------------------------------
 a      | integer |           |          |
 b      | integer |           |          | generated always as (10) stored
Inherits: p2

Also, CREATE TABLE doesn't support to create a generated column on
inherited table, which is the same name but is a normal column on the
parent table, as follows:

=# create table p3 (a int, b int);
=# create table c3 (a int, b int generated always as (a + 2) stored)
inherits(p3);
ERROR:  cannot use column reference in DEFAULT expression
LINE 1: ...reate table c3 (a int, b int generated always as (a + 2) sto...

Aside from the error message seems not correct, it's actually possible
that we can have only the inherited table's column have a generation
expression by:

=# create table p4 (a int, b int);
=# create table c4 (a int);
=# alter table c4 add column b int generated always as (a * 3) stored;
=# alter table c4 inherit p4;

Because of this behavior, pg_dump generates a query for the table c4
that cannot be restored.

I think we can fix these issues with the attached patch but it seems
better discussing the desired behavior first.

Regards,

[1] https://www.postgresql.org/message-id/2678bad1-048f-519a-ef24-b12962f41807@enterprisedb.com


--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-04-23 08:35, Masahiko Sawada wrote:
> After investigating this issue, I think that current DDLs regarding
> inherited tables and generated columns seem not to work fine.

Right, there were a number of combinations that were not properly 
handled.  The attached patch should fix them all.  It's made against 
PG12 but also works on master.  See contained commit message and 
documentation for details.

(This does not touch the issues with pg_dump, but it helps clarify the 
cases that pg_dump needs to handle.)

> We can make an inherited table have the same column having a different
> generation expression as follows:
> 
> =# create table p1 (a int, b int generated always as (a + 1) stored);
> =# create table c1 (a int, b int generated always as (a + 2) stored)
> inherits(p1);

With my patch, this becomes an error.

> But the column on the inherited table has a default value, the column
> will be generation expression with a const value:
> 
> =# create table p2 (a int, b int generated always as (a + 1) stored);
> =# create table c2 (a int, b int default 10) inherits(p2);
> =# \d c2
>                               Table "public.c2"
>   Column |  Type   | Collation | Nullable |             Default
> --------+---------+-----------+----------+---------------------------------
>   a      | integer |           |          |
>   b      | integer |           |          | generated always as (10) stored
> Inherits: p2

With my patch, this also becomes an error.

> Also, CREATE TABLE doesn't support to create a generated column on
> inherited table, which is the same name but is a normal column on the
> parent table, as follows:
> 
> =# create table p3 (a int, b int);
> =# create table c3 (a int, b int generated always as (a + 2) stored)
> inherits(p3);
> ERROR:  cannot use column reference in DEFAULT expression
> LINE 1: ...reate table c3 (a int, b int generated always as (a + 2) sto...

This is allowed with my patch (which is basically an expanded version of 
your patch).

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-05-06 16:29, Peter Eisentraut wrote:
> On 2020-04-23 08:35, Masahiko Sawada wrote:
>> After investigating this issue, I think that current DDLs regarding
>> inherited tables and generated columns seem not to work fine.
> 
> Right, there were a number of combinations that were not properly
> handled.  The attached patch should fix them all.  It's made against
> PG12 but also works on master.  See contained commit message and
> documentation for details.

committed to master and PG12

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> Right, there were a number of combinations that were not properly
>> handled.  The attached patch should fix them all.  It's made against
>> PG12 but also works on master.  See contained commit message and
>> documentation for details.

> committed to master and PG12

So ... this did not actually fix the dump/restore problem.  In fact,
it's worse, because in HEAD I see two failures not one when doing the
same test proposed at the start of this thread:

1. make installcheck
2. pg_dump -Fc regression >r.dump
3. createdb r2
4. pg_restore -d r2 r.dump

pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 6253; 2604 226187 DEFAULT gtest1_1 b postgres
pg_restore: error: could not execute query: ERROR:  column "b" of relation "gtest1_1" is a generated column
Command was: ALTER TABLE ONLY public.gtest1_1 ALTER COLUMN b SET DEFAULT (a * 2);


pg_restore: from TOC entry 6279; 2604 227276 DEFAULT gtest30_1 b postgres
pg_restore: error: could not execute query: ERROR:  cannot use column reference in DEFAULT expression
Command was: ALTER TABLE ONLY public.gtest30_1 ALTER COLUMN b SET DEFAULT (a * 2);


pg_restore: warning: errors ignored on restore: 2

            regards, tom lane

On Thu, 16 Jul 2020 at 04:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> >> Right, there were a number of combinations that were not properly
> >> handled.  The attached patch should fix them all.  It's made against
> >> PG12 but also works on master.  See contained commit message and
> >> documentation for details.
>
> > committed to master and PG12
>
> So ... this did not actually fix the dump/restore problem.  In fact,
> it's worse, because in HEAD I see two failures not one when doing the
> same test proposed at the start of this thread:
>
> 1. make installcheck
> 2. pg_dump -Fc regression >r.dump
> 3. createdb r2
> 4. pg_restore -d r2 r.dump
>
> pg_restore: while PROCESSING TOC:
> pg_restore: from TOC entry 6253; 2604 226187 DEFAULT gtest1_1 b postgres
> pg_restore: error: could not execute query: ERROR:  column "b" of relation "gtest1_1" is a generated column
> Command was: ALTER TABLE ONLY public.gtest1_1 ALTER COLUMN b SET DEFAULT (a * 2);
>
>
> pg_restore: from TOC entry 6279; 2604 227276 DEFAULT gtest30_1 b postgres
> pg_restore: error: could not execute query: ERROR:  cannot use column reference in DEFAULT expression
> Command was: ALTER TABLE ONLY public.gtest30_1 ALTER COLUMN b SET DEFAULT (a * 2);
>
>
> pg_restore: warning: errors ignored on restore: 2
>

The minimum reproducer is:

create table a (a int, b int generated always as (a * 2) stored);
create table aa () inherits (a);

pg_dump produces the following DDLs:

CREATE TABLE public.a (
    a integer,
    b integer GENERATED ALWAYS AS ((a * 2)) STORED
);

CREATE TABLE public.aa (
)
INHERITS (public.a);

ALTER TABLE ONLY public.aa ALTER COLUMN b SET DEFAULT (a * 2);

However, the ALTER TABLE fails.

By commit 086ffddf, the child tables must have the same generation
expression as the expression defined in the parent. So I think pg_dump
should not generate the last DDL. I've attached the patch fixing this
issue.

Apart from the fix, I wonder if we can add a test that dumps the
database where executed 'make check' and restore it to another
database.

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, 23 Jul 2020 at 19:55, Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Thu, 16 Jul 2020 at 04:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> > >> Right, there were a number of combinations that were not properly
> > >> handled.  The attached patch should fix them all.  It's made against
> > >> PG12 but also works on master.  See contained commit message and
> > >> documentation for details.
> >
> > > committed to master and PG12
> >
> > So ... this did not actually fix the dump/restore problem.  In fact,
> > it's worse, because in HEAD I see two failures not one when doing the
> > same test proposed at the start of this thread:
> >
> > 1. make installcheck
> > 2. pg_dump -Fc regression >r.dump
> > 3. createdb r2
> > 4. pg_restore -d r2 r.dump
> >
> > pg_restore: while PROCESSING TOC:
> > pg_restore: from TOC entry 6253; 2604 226187 DEFAULT gtest1_1 b postgres
> > pg_restore: error: could not execute query: ERROR:  column "b" of relation "gtest1_1" is a generated column
> > Command was: ALTER TABLE ONLY public.gtest1_1 ALTER COLUMN b SET DEFAULT (a * 2);
> >
> >
> > pg_restore: from TOC entry 6279; 2604 227276 DEFAULT gtest30_1 b postgres
> > pg_restore: error: could not execute query: ERROR:  cannot use column reference in DEFAULT expression
> > Command was: ALTER TABLE ONLY public.gtest30_1 ALTER COLUMN b SET DEFAULT (a * 2);
> >
> >
> > pg_restore: warning: errors ignored on restore: 2
> >
>
> The minimum reproducer is:
>
> create table a (a int, b int generated always as (a * 2) stored);
> create table aa () inherits (a);
>
> pg_dump produces the following DDLs:
>
> CREATE TABLE public.a (
>     a integer,
>     b integer GENERATED ALWAYS AS ((a * 2)) STORED
> );
>
> CREATE TABLE public.aa (
> )
> INHERITS (public.a);
>
> ALTER TABLE ONLY public.aa ALTER COLUMN b SET DEFAULT (a * 2);
>
> However, the ALTER TABLE fails.
>
> By commit 086ffddf, the child tables must have the same generation
> expression as the expression defined in the parent. So I think pg_dump
> should not generate the last DDL. I've attached the patch fixing this
> issue.
>
> Apart from the fix, I wonder if we can add a test that dumps the
> database where executed 'make check' and restore it to another
> database.
>

This issue is not fixed yet. I've attached the updated version patch
and registered it to commit fest so as not to forget. Please review
it.

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I have been analyzing this issue again.  We have a few candidate patches 
that do very similar things for avoiding dumping the generation 
expression of table gtest1_1.  We can figure out later which one of 
these we like best.  But there is another issue lurking nearby.  The 
table hierarchy of gtest30, which is created in the regression tests 
like this:

CREATE TABLE gtest30 (
     a int,
     b int GENERATED ALWAYS AS (a * 2) STORED
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
ALTER TABLE ONLY gtest30 ALTER COLUMN b DROP EXPRESSION;

This drops the generation expression from the parent table but not the 
child table.  This is currently dumped like this:

CREATE TABLE public.gtest30 (
     a integer,
     b integer
);

CREATE TABLE public.gtest30_1 (
)
INHERITS (public.gtest30);

ALTER TABLE ONLY public.gtest30_1 ALTER COLUMN b SET DEFAULT (a * 2);

The proposed patches will cause the last statement to be omitted, but 
that still won't recreate the original state.  The problem is that there 
is no command to make a column generated afterwards, like the SET 
DEFAULT command, so we can't dump it like this.  We would have to produce

CREATE TABLE public.gtest30 (
     a integer,
     b integer
);

CREATE TABLE public.gtest30_1 (
     b integer GENERATED ALWAYS AS (a * 2) STORED
)
INHERITS (public.gtest30);

but this will create the column "b" of gtest30_1 as attlocal, which the 
original command sequence does not.

We could probably fix this by having ALTER TABLE ONLY / DROP EXPRESSION 
update the attlocal column of direct children to true, to make the 
catalog state look like something that can be restored.  However, that's 
a fair amount of complicated code, so for now I propose to just prohibit 
this command, meaning you can't use ONLY in this command if there are 
children.  This is new in PG13, so this change would have very limited 
impact in practice.

Proposed patch attached.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On 25 Sep 2020, at 15:07, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:

> We could probably fix this by having ALTER TABLE ONLY / DROP EXPRESSION update the attlocal column of direct children
totrue, to make the catalog state look like something that can be restored.  However, that's a fair amount of
complicatedcode, so for now I propose to just prohibit this command, meaning you can't use ONLY in this command if
thereare children.  This is new in PG13, so this change would have very limited impact in practice. 

That sounds a bit dramatic.  Do you propose to do that in v13 as well or just
in HEAD?  If the latter, considering that the window until the 14 freeze is
quite wide shouldn't we try to fix it first?

cheers ./daniel

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> The proposed patches will cause the last statement to be omitted, but 
> that still won't recreate the original state.  The problem is that there 
> is no command to make a column generated afterwards, like the SET 
> DEFAULT command, so we can't dump it like this.

Right.  I'm not even sure what such a command should do ... would it run
through all existing rows and update them to be the GENERATED value?

> We could probably fix this by having ALTER TABLE ONLY / DROP EXPRESSION 
> update the attlocal column of direct children to true, to make the 
> catalog state look like something that can be restored.  However, that's 
> a fair amount of complicated code, so for now I propose to just prohibit 
> this command, meaning you can't use ONLY in this command if there are 
> children.  This is new in PG13, so this change would have very limited 
> impact in practice.

+1.  At this point we would want some fairly un-complicated fix for
the v13 branch anyhow, and this seems to fit the bill.  (Also, having
child columns suddenly grow an attislocal property doesn't seem to meet
the principle of least surprise.)

            regards, tom lane

On Fri, 25 Sep 2020 at 22:07, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
>
> I have been analyzing this issue again.  We have a few candidate patches
> that do very similar things for avoiding dumping the generation
> expression of table gtest1_1.  We can figure out later which one of
> these we like best.  But there is another issue lurking nearby.  The
> table hierarchy of gtest30, which is created in the regression tests
> like this:
>
> CREATE TABLE gtest30 (
>      a int,
>      b int GENERATED ALWAYS AS (a * 2) STORED
> );
> CREATE TABLE gtest30_1 () INHERITS (gtest30);
> ALTER TABLE ONLY gtest30 ALTER COLUMN b DROP EXPRESSION;
>
> This drops the generation expression from the parent table but not the
> child table.  This is currently dumped like this:
>
> CREATE TABLE public.gtest30 (
>      a integer,
>      b integer
> );
>
> CREATE TABLE public.gtest30_1 (
> )
> INHERITS (public.gtest30);
>
> ALTER TABLE ONLY public.gtest30_1 ALTER COLUMN b SET DEFAULT (a * 2);
>
> The proposed patches will cause the last statement to be omitted, but
> that still won't recreate the original state.  The problem is that there
> is no command to make a column generated afterwards, like the SET
> DEFAULT command, so we can't dump it like this.  We would have to produce
>
> CREATE TABLE public.gtest30 (
>      a integer,
>      b integer
> );
>
> CREATE TABLE public.gtest30_1 (
>      b integer GENERATED ALWAYS AS (a * 2) STORED
> )
> INHERITS (public.gtest30);
>
> but this will create the column "b" of gtest30_1 as attlocal, which the
> original command sequence does not.
>
> We could probably fix this by having ALTER TABLE ONLY / DROP EXPRESSION
> update the attlocal column of direct children to true, to make the
> catalog state look like something that can be restored.  However, that's
> a fair amount of complicated code, so for now I propose to just prohibit
> this command, meaning you can't use ONLY in this command if there are
> children.  This is new in PG13, so this change would have very limited
> impact in practice.
>
> Proposed patch attached.

+1

If we have ALTER TABLE ONLY / DROP EXPRESSION update the attlocal
column of children to true to fix the issue you raised, my proposed
patch is not necessary. OTOH if we fix it by prohibiting the command,
I guess we need both patches to fix the issues.

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

[ Pulling Daniel into this older thread seems like the cleanest way to
  unify the two threads ]

Masahiko Sawada <masahiko.sawada@2ndquadrant.com> writes:
> If we have ALTER TABLE ONLY / DROP EXPRESSION update the attlocal
> column of children to true to fix the issue you raised, my proposed
> patch is not necessary. OTOH if we fix it by prohibiting the command,
> I guess we need both patches to fix the issues.

Right, Peter already mentioned that we need a further pg_dump fix on
top of prohibiting the ONLY ... DROP EXPRESSION case.

Bug #16622 [1] is another report of this same issue, and in that thread,
Daniel argues that the right fix is just

+    /*
+     * Skip if the column isn't local to the table's definition as the attrdef
+     * will be printed with the inheritance parent table definition
+     */
+    if (!tbinfo->attislocal[adnum - 1])
+        return;

without the attgenerated clause that Masahiko-san proposes.
I think however that that's wrong.  It is possible to have
a non-attislocal column that has its own default, because
you can do

d3=# create table parent (f1 int default 2);
CREATE TABLE
d3=# create table child (f1 int default 3) inherits(parent);
NOTICE:  merging column "f1" with inherited definition
CREATE TABLE
d3=# create table child2() inherits(parent);
CREATE TABLE
d3=# alter table child2 alter column f1 set default 42;
ALTER TABLE

This does not cause child2.f1's attislocal property to become
true.  Maybe it should have, but it's probably too late for
that; at least, pg_dump couldn't assume it's true in older
servers.  Therefore, since we can't tell whether the default
is inherited or not, we'd better dump it.

(I recall that pg_dump has a hack somewhere that checks for
textual equality of CHECK conditions to avoid dumping redundant
child copies.  Maybe we could do something similar here.)

The situation is different for GENERATED columns, since we disallow
a child having a different GENERATED property than the parent.
However, I think Masahiko-san's patch is not quite right either,
because a column can be both inherited and local.  An example is

d3=# create table pgen (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
CREATE TABLE
d3=# create table cgen1 (b int) inherits (pgen);
NOTICE:  moving and merging column "b" with inherited definition
DETAIL:  User-specified column moved to the position of the inherited column.
CREATE TABLE
d3=# select attname, attislocal, attinhcount from pg_attribute where attrelid = 'cgen1'::regclass and attnum>0;
 attname | attislocal | attinhcount
---------+------------+-------------
 a       | f          |           1
 b       | t          |           1
(2 rows)

So it appears to me that a more correct coding is

     /*
      * Do not print a GENERATED default for an inherited column; it will
      * be inherited from the parent, and the backend won't accept a
      * command to set it separately.
      */
     if (tbinfo->attinhcount[adnum - 1] > 0 && tbinfo->attgenerated[adnum - 1])
         return;

Unfortunately this has still got a problem: it will mishandle the case of
a child column that is GENERATED while its parent is not.  Peter opined
way upthread that we should not allow that, but according to my testing
we do.

This'd all be a lot cleaner if defaults were marked as to whether they
were inherited or locally generated.  Maybe we ought to work on that.

In the meantime, maybe the best bet is for pg_dump to try to detect
whether a default is identical to a parent default, and not dump it
if so.  That would fix both the GENERATED case where we must not
dump it, and the non-GENERATED case where it's merely inefficient
to do so.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/16622-779a79851b4e2491%40postgresql.org

I wrote:
> The situation is different for GENERATED columns, since we disallow
> a child having a different GENERATED property than the parent.

BTW, that alleged prohibition is pretty damn leaky:

d3=# create table pp1 (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
CREATE TABLE
d3=# create table cc1 (a int, b int GENERATED ALWAYS AS (a * 3) STORED);
CREATE TABLE
d3=# alter table cc1 inherit pp1;
ALTER TABLE

Maybe the *real* fix here is to give up on this idea that they
can't be different?

            regards, tom lane

> On 29 Sep 2020, at 18:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> [ Pulling Daniel into this older thread seems like the cleanest way to
>  unify the two threads ]
>
> Masahiko Sawada <masahiko.sawada@2ndquadrant.com> writes:
>> If we have ALTER TABLE ONLY / DROP EXPRESSION update the attlocal
>> column of children to true to fix the issue you raised, my proposed
>> patch is not necessary. OTOH if we fix it by prohibiting the command,
>> I guess we need both patches to fix the issues.
>
> Right, Peter already mentioned that we need a further pg_dump fix on
> top of prohibiting the ONLY ... DROP EXPRESSION case.
>
> Bug #16622 [1] is another report of this same issue, and in that thread,
> Daniel argues that the right fix is just
>
> +    /*
> +     * Skip if the column isn't local to the table's definition as the attrdef
> +     * will be printed with the inheritance parent table definition
> +     */
> +    if (!tbinfo->attislocal[adnum - 1])
> +        return;
>
> without the attgenerated clause that Masahiko-san proposes.
> I think however that that's wrong.  It is possible to have
> a non-attislocal column that has its own default, because
> you can do

Ah, that's the sequence I didn't think of.  I agree with your assessment of
this being wrong. Thanks!

> This does not cause child2.f1's attislocal property to become
> true.  Maybe it should have, but it's probably too late for
> that; at least, pg_dump couldn't assume it's true in older
> servers.

Do you recall the rationale for it not being set to true?  I didn't spot
anything in the commit history. Intuitively it seems a tad odd.

> Therefore, since we can't tell whether the default
> is inherited or not, we'd better dump it.

Agreed.

> (I recall that pg_dump has a hack somewhere that checks for
> textual equality of CHECK conditions to avoid dumping redundant
> child copies.  Maybe we could do something similar here.)

Unless someone beats me to it I will take a stab at this to see what it would
look like.

cheers ./daniel

Daniel Gustafsson <daniel@yesql.se> writes:
> On 29 Sep 2020, at 18:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This does not cause child2.f1's attislocal property to become
>> true.  Maybe it should have, but it's probably too late for
>> that; at least, pg_dump couldn't assume it's true in older
>> servers.  

> Do you recall the rationale for it not being set to true?  I didn't spot
> anything in the commit history. Intuitively it seems a tad odd.

I'd bet the explanation is mostly that it didn't occur to anyone
that SET DEFAULT should do that.  I'm not really proposing that it
should either.  If we were to make any catalog changes in response
to this, what I'd vote for is to add an "inherited" flag to
pg_attrdef.  (I'm not quite sure if a bool would be sufficient,
or if we'd need to go to the same extent as pg_attribute does,
and have a bool plus an inheritance count.)

Of course, that line of thought does not lead to anything
back-patchable.  But pg_dump would have to be prepared to cope
with the situation in older servers in any case.

            regards, tom lane

On 2020-09-29 18:37, Tom Lane wrote:
> Unfortunately this has still got a problem: it will mishandle the case of
> a child column that is GENERATED while its parent is not.  Peter opined
> way upthread that we should not allow that, but according to my testing
> we do.

Did I opine that?  Commit 086ffddf3656fb3d24d9a73ce36cb1102e42cc90 
explicitly allowed that case.  What we don't want is the other way around.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-09-25 15:07, Peter Eisentraut wrote:
> We could probably fix this by having ALTER TABLE ONLY / DROP EXPRESSION
> update the attlocal column of direct children to true, to make the
> catalog state look like something that can be restored.  However, that's
> a fair amount of complicated code, so for now I propose to just prohibit
> this command, meaning you can't use ONLY in this command if there are
> children.  This is new in PG13, so this change would have very limited
> impact in practice.

With the minor releases coming up, I have committed this patch and will 
work on getting the remaining pg_dump issues fixed as well.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-08-27 13:30, Masahiko Sawada wrote:
> This issue is not fixed yet. I've attached the updated version patch
> and registered it to commit fest so as not to forget. Please review
> it.

A few fixes have been committed in this thread, basically to prevent 
situations that shouldn't have been allowed.

What's left is the originally reported issue that some parts of the 
regression test database are dumped incorrectly.  The two proposed 
patches in their most recent versions are

https://www.postgresql.org/message-id/attachment/107447/v3-0001-pg_dump-Fix-dumping-of-inherited-generated-column.patch

(message 
https://www.postgresql.org/message-id/b1c831dd-d520-5e7f-0304-0eeed39c9996%402ndquadrant.com)

and

https://www.postgresql.org/message-id/attachment/113487/fix_gcolumn_dump_v2.patch 
(message 
https://www.postgresql.org/message-id/CA%2Bfd4k6pLzrZDQsdsxcS06AwGRf1DgwOw84sFq9oXNw%2B83nB1g%40mail.gmail.com)

Both of these result in the same change to the dump output.  Both of 
them have essentially the same idea.  The first one adds the 
conditionals during the information gathering phase of pg_dump, the 
second one adds the conditionals during the output phase.

Any further thoughts?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Nov 4, 2020 at 4:23 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
>
> On 2020-08-27 13:30, Masahiko Sawada wrote:
> > This issue is not fixed yet. I've attached the updated version patch
> > and registered it to commit fest so as not to forget. Please review
> > it.
>
> A few fixes have been committed in this thread, basically to prevent
> situations that shouldn't have been allowed.
>
> What's left is the originally reported issue that some parts of the
> regression test database are dumped incorrectly.  The two proposed
> patches in their most recent versions are
>
>
https://www.postgresql.org/message-id/attachment/107447/v3-0001-pg_dump-Fix-dumping-of-inherited-generated-column.patch
> (message
> https://www.postgresql.org/message-id/b1c831dd-d520-5e7f-0304-0eeed39c9996%402ndquadrant.com)
>
> and
>
> https://www.postgresql.org/message-id/attachment/113487/fix_gcolumn_dump_v2.patch
> (message
> https://www.postgresql.org/message-id/CA%2Bfd4k6pLzrZDQsdsxcS06AwGRf1DgwOw84sFq9oXNw%2B83nB1g%40mail.gmail.com)
>
> Both of these result in the same change to the dump output.  Both of
> them have essentially the same idea.  The first one adds the
> conditionals during the information gathering phase of pg_dump, the
> second one adds the conditionals during the output phase.
>
> Any further thoughts?

I think the first one is better than the second (mine) because it can
save the number of intermediate objects.

Regards,


--
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/

On 2020-11-06 04:55, Masahiko Sawada wrote:
>> Both of these result in the same change to the dump output.  Both of
>> them have essentially the same idea.  The first one adds the
>> conditionals during the information gathering phase of pg_dump, the
>> second one adds the conditionals during the output phase.
>>
>> Any further thoughts?
> I think the first one is better than the second (mine) because it can
> save the number of intermediate objects.

I was hoping to wrap this issue up this week, but I found more problems 
with how these proposed changes interact with --binary-upgrade mode.  I 
think I need to formalize my findings into pg_dump test cases as a next 
step.  Then we can figure out what combination of tweaks will make them 
all work.

-- 
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

On 09.11.2020 13:43, Peter Eisentraut wrote:
> On 2020-11-06 04:55, Masahiko Sawada wrote:
>>> Both of these result in the same change to the dump output.  Both of
>>> them have essentially the same idea.  The first one adds the
>>> conditionals during the information gathering phase of pg_dump, the
>>> second one adds the conditionals during the output phase.
>>>
>>> Any further thoughts?
>> I think the first one is better than the second (mine) because it can
>> save the number of intermediate objects.
>
> I was hoping to wrap this issue up this week, but I found more 
> problems with how these proposed changes interact with 
> --binary-upgrade mode.  I think I need to formalize my findings into 
> pg_dump test cases as a next step.  Then we can figure out what 
> combination of tweaks will make them all work.
>
I am moving this patch to the next CF, but it looks like the discussion 
is a bit stuck.


Peter, can you please share your concerns about the interaction of the 
patch with --binary-upgrade mode? If you don't have time to write tests, 
you can just describe problems.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

I've had another go at this, and I've found a solution that appears to 
address all the issues I'm aware of.  It's all very similar to the 
previously discussed patches.  The main difference is that previous 
patches had attempted to use something like tbinfo->attislocal to 
determine whether a column was inherited, but that's not correct.  This 
patch uses the existing logic in flagInhAttrs() to find whether there is 
a matching parent column with a generation expression.  I've added 
pg_dump test cases here to check the different variations that the code 
addresses.

-- 
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> I've had another go at this, and I've found a solution that appears to 
> address all the issues I'm aware of.  It's all very similar to the 
> previously discussed patches.  The main difference is that previous 
> patches had attempted to use something like tbinfo->attislocal to 
> determine whether a column was inherited, but that's not correct.  This 
> patch uses the existing logic in flagInhAttrs() to find whether there is 
> a matching parent column with a generation expression.  I've added 
> pg_dump test cases here to check the different variations that the code 
> addresses.

This is a clear improvement on the current situation, and given that
this issue is over a year old, I think we should push and back-patch
this in time for February's releases.

However ... this doesn't solve all the cases noted in this thread.
In the first example I gave at [1],

d3=# create table parent (f1 int default 2);
CREATE TABLE
d3=# create table child (f1 int default 3) inherits(parent);
NOTICE:  merging column "f1" with inherited definition
CREATE TABLE
d3=# create table child2() inherits(parent);
CREATE TABLE
d3=# alter table child2 alter column f1 set default 42;
ALTER TABLE

pg_dump still fails to restore child2.f1's non-inherited default.
That's probably a pre-existing problem, since it doesn't involve
GENERATED at all, but we shouldn't forget about it.

Also, in the example from [2],

d3=# create table pp1 (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
CREATE TABLE
d3=# create table cc1 (a int, b int GENERATED ALWAYS AS (a * 3) STORED);
CREATE TABLE
d3=# alter table cc1 inherit pp1;
ALTER TABLE

pg_dump now omits to dump cc1's generation expression, which seems
strictly worse than before.  Admittedly, the backend likely ought to
be rejecting this scenario, but it doesn't do so today.

Neither of these points seem like a reason to reject this patch,
they're just adjacent work that remains to be done.

            regards, tom lane

[1] https://www.postgresql.org/message-id/660925.1601397436%40sss.pgh.pa.us
[2] https://www.postgresql.org/message-id/661371.1601398006%40sss.pgh.pa.us

On 2021-01-29 17:41, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> I've had another go at this, and I've found a solution that appears to
>> address all the issues I'm aware of.  It's all very similar to the
>> previously discussed patches.  The main difference is that previous
>> patches had attempted to use something like tbinfo->attislocal to
>> determine whether a column was inherited, but that's not correct.  This
>> patch uses the existing logic in flagInhAttrs() to find whether there is
>> a matching parent column with a generation expression.  I've added
>> pg_dump test cases here to check the different variations that the code
>> addresses.
> 
> This is a clear improvement on the current situation, and given that
> this issue is over a year old, I think we should push and back-patch
> this in time for February's releases.

done

I will continue working on the other issues that we have been discussing.

> However ... this doesn't solve all the cases noted in this thread.
> In the first example I gave at [1],
> 
> d3=# create table parent (f1 int default 2);
> CREATE TABLE
> d3=# create table child (f1 int default 3) inherits(parent);
> NOTICE:  merging column "f1" with inherited definition
> CREATE TABLE
> d3=# create table child2() inherits(parent);
> CREATE TABLE
> d3=# alter table child2 alter column f1 set default 42;
> ALTER TABLE
> 
> pg_dump still fails to restore child2.f1's non-inherited default.
> That's probably a pre-existing problem, since it doesn't involve
> GENERATED at all, but we shouldn't forget about it.
> 
> Also, in the example from [2],
> 
> d3=# create table pp1 (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
> CREATE TABLE
> d3=# create table cc1 (a int, b int GENERATED ALWAYS AS (a * 3) STORED);
> CREATE TABLE
> d3=# alter table cc1 inherit pp1;
> ALTER TABLE
> 
> pg_dump now omits to dump cc1's generation expression, which seems
> strictly worse than before.  Admittedly, the backend likely ought to
> be rejecting this scenario, but it doesn't do so today.
> 
> Neither of these points seem like a reason to reject this patch,
> they're just adjacent work that remains to be done.
> 
>             regards, tom lane
> 
> [1] https://www.postgresql.org/message-id/660925.1601397436%40sss.pgh.pa.us
> [2] https://www.postgresql.org/message-id/661371.1601398006%40sss.pgh.pa.us
> 
> 


-- 
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

On 2021-01-29 17:41, Tom Lane wrote:
> However ... this doesn't solve all the cases noted in this thread.
> In the first example I gave at [1],
> 
> d3=# create table parent (f1 int default 2);
> CREATE TABLE
> d3=# create table child (f1 int default 3) inherits(parent);
> NOTICE:  merging column "f1" with inherited definition
> CREATE TABLE
> d3=# create table child2() inherits(parent);
> CREATE TABLE
> d3=# alter table child2 alter column f1 set default 42;
> ALTER TABLE
> 
> pg_dump still fails to restore child2.f1's non-inherited default.
> That's probably a pre-existing problem, since it doesn't involve
> GENERATED at all, but we shouldn't forget about it.

> [1] https://www.postgresql.org/message-id/660925.1601397436%40sss.pgh.pa.us

I can't tell what the problem is in this example.  I tried with PG11, 
12, and master, and the schema dump comes out with those same four 
commands and they restore correctly AFAICT.

-- 
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2021-01-29 17:41, Tom Lane wrote:
>> However ... this doesn't solve all the cases noted in this thread.
>> In the first example I gave at [1],
>> d3=# create table parent (f1 int default 2);
>> CREATE TABLE
>> d3=# create table child (f1 int default 3) inherits(parent);
>> NOTICE:  merging column "f1" with inherited definition
>> CREATE TABLE
>> d3=# create table child2() inherits(parent);
>> CREATE TABLE
>> d3=# alter table child2 alter column f1 set default 42;
>> ALTER TABLE
>> 
>> pg_dump still fails to restore child2.f1's non-inherited default.

> I can't tell what the problem is in this example.  I tried with PG11, 
> 12, and master, and the schema dump comes out with those same four 
> commands and they restore correctly AFAICT.

Oh!  Trying it now, I see that the child2 default does get restored
as a "separate default" object:

ALTER TABLE ONLY public.child2 ALTER COLUMN f1 SET DEFAULT 42;

This is a bit weird, because you'd think it would be handled
the same as the other child's default, but it isn't; that
one comes out as

CREATE TABLE public.child (
    f1 integer DEFAULT 3
)
INHERITS (public.parent);

while child2 looks like

CREATE TABLE public.child2 (
)
INHERITS (public.parent);


I now suspect that I'd seen this dump of "child2" and missed the later
ALTER.  So no bug here, just pilot error.  Sorry for the noise.

            regards, tom lane

On 2021-01-29 17:41, Tom Lane wrote:
> Also, in the example from [2],
> 
> d3=# create table pp1 (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
> CREATE TABLE
> d3=# create table cc1 (a int, b int GENERATED ALWAYS AS (a * 3) STORED);
> CREATE TABLE
> d3=# alter table cc1 inherit pp1;
> ALTER TABLE
> 
> pg_dump now omits to dump cc1's generation expression, which seems
> strictly worse than before.  Admittedly, the backend likely ought to
> be rejecting this scenario, but it doesn't do so today.
> 
> [2]https://www.postgresql.org/message-id/661371.1601398006%40sss.pgh.pa.us

Here is a WIP patch to address this.  Probably needs another look for 
column number mapping and all the usual stuff, but the basic idea should 
be okay.

-- 
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

On 2021-02-04 01:17, Zhihong Yu wrote:
> Hi,
> +           if (attribute->attgenerated && !childatt->attgenerated)
> +               ereport(ERROR,
> ...
> +           if (attribute->attgenerated && childatt->attgenerated)
> +           {
> 
> Looks like for the second if statement, 
> checking attribute->attgenerated should be enough (due to the check from 
> the first if statement).

Thanks for taking a look.  I figured the way I wrote it makes it easier 
to move the code around or insert other code in the future and doesn't 
make it so tightly coupled.

Anyway, I figured out how to take account of generation expressions with 
different column orders.  I used the same approach that we use for check 
constraints.  The attached patch is good to go from my perspective.

-- 
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

On 05.02.21 15:18, Peter Eisentraut wrote:
> Anyway, I figured out how to take account of generation expressions with 
> different column orders.  I used the same approach that we use for check 
> constraints.  The attached patch is good to go from my perspective.

Dusting this off ... this patch should go into the next minor releases. 
The attached patch is for master but backpatches without manual 
intervention to PG13 and PG12.

On 26.04.21 14:10, Peter Eisentraut wrote:
> On 05.02.21 15:18, Peter Eisentraut wrote:
>> Anyway, I figured out how to take account of generation expressions 
>> with different column orders.  I used the same approach that we use 
>> for check constraints.  The attached patch is good to go from my 
>> perspective.
> 
> Dusting this off ... this patch should go into the next minor releases. 
> The attached patch is for master but backpatches without manual 
> intervention to PG13 and PG12.

committed