Dumping/restoring fails on inherited generated column

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]/messages/by-id/2678bad1-048f-519a-ef24-b12962f41807@enterprisedb.com.

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]: /messages/by-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 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.

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]/messages/by-id/16622-779a79851b4e2491@postgresql.org 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]: /messages/by-id/16622-779a79851b4e2491@postgresql.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