Обсуждение: BUG #16622: pg_dump produces erroneus ALTER TABLE statement for a table with an inherited generated column

The following bug has been logged on the website:

Bug reference:      16622
Logged by:          Andrew Bille
Email address:      andrewbille@gmail.com
PostgreSQL version: 12.4
Operating system:   Ubuntu-20.04
Description:

When I perform dump/reload with the following inherited table:

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

CREATE TABLE public.gtest1_1 (
)
INHERITS (public.gtest1);

I get the error:

pg_dump postgres > dump.sql
createdb test
psql test -q -v ON_ERROR_STOP=1 -f dump.sql
psql:dump.sql:50: ERROR:  column "b" of relation "gtest1_1" is a generated
column

in dump.sql i see
...
--
-- Name: gtest1; Type: TABLE; Schema: public; Owner: postgres
--

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


ALTER TABLE public.gtest1 OWNER TO postgres;

--
-- Name: gtest1_1; Type: TABLE; Schema: public; Owner: postgres
--

CREATE TABLE public.gtest1_1 (
)
INHERITS (public.gtest1);


ALTER TABLE public.gtest1_1 OWNER TO postgres;

--
-- Name: gtest1_1 b; Type: DEFAULT; Schema: public; Owner: postgres
--

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

Reproduced on 13rc1 too.

> On 18 Sep 2020, at 06:58, PG Bug reporting form <noreply@postgresql.org> wrote:

> psql:dump.sql:50: ERROR:  column "b" of relation "gtest1_1" is a generated
> column

I can reproduce this in git HEAD too.  Off the cuff, ISTM that we should only
dump attrdefs for local columns during non-upgrade dumps?  It works for normal
DEFAULTs but the restored db will have different :location for the attrdef.
For generated columns it errors out as shown above.  I don't really follow the
reason for why we redo the normal DEFAULT for non-local columns today, is that
intentional?

The attached, potentially naive, patch fixes the above issue and passes pg_dump
and pg_upgrade tests.

cheers ./daniel

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 18 Sep 2020, at 06:58, PG Bug reporting form <noreply@postgresql.org> wrote:
>> psql:dump.sql:50: ERROR:  column "b" of relation "gtest1_1" is a generated
>> column

> I can reproduce this in git HEAD too.

Yeah, this has been complained of before, notably at [1].
The latest patch in that thread is Masahiko-san's at [2],
which is similar to but not exactly like yours.
Could you review/test that one?

(I suspect you are right that binary-upgrade is a special case,
but wouldn't the existing pg_upgrade test catch that?  Also,
do we need more test cases?)

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/15830.1575468847%40sss.pgh.pa.us
[2] https://www.postgresql.org/message-id/CA%2Bfd4k6pLzrZDQsdsxcS06AwGRf1DgwOw84sFq9oXNw%2B83nB1g%40mail.gmail.com

> On 21 Sep 2020, at 23:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 18 Sep 2020, at 06:58, PG Bug reporting form <noreply@postgresql.org> wrote:
>>> psql:dump.sql:50: ERROR:  column "b" of relation "gtest1_1" is a generated
>>> column
>
>> I can reproduce this in git HEAD too.
>
> Yeah, this has been complained of before, notably at [1].
> The latest patch in that thread is Masahiko-san's at [2],
> which is similar to but not exactly like yours.
> Could you review/test that one?

Aha, thanks for pointing it out, I had missed that thread.  Looking at the
patch in that thread, we have come to the same conclusion independently.  Both
patches handle attrdefs from generated columms in the same way, they do however
differ for other non-local attrdefs.  Consider the following:

  CREATE TABLE public.gtest2 (b integer DEFAULT 2);
  CREATE TABLE public.gtest2_2 () INHERITS (public.gtest2);

Masahiko-san's patch (and current HEAD) will dump the following (blank rows
omitted for brevity), as will HEAD.  Note the ALTER TABLE ..  SET DEFAULT at
the end.

  CREATE TABLE public.gtest2 (
      b integer DEFAULT 2
  );
  ALTER TABLE public.gtest2 OWNER TO dgustafsson;
  CREATE TABLE public.gtest2_2 (
  )
  INHERITS (public.gtest2);
  ALTER TABLE public.gtest2_2 OWNER TO dgustafsson;
  ALTER TABLE ONLY public.gtest2_2 ALTER COLUMN b SET DEFAULT 2;

Is there is a reason for performing that step, it will drop the existing
attrdef and install an exact copy.  The attached path will dump the same
sequence of operations but without the SET DEFAULT command:

  CREATE TABLE public.gtest2 (
      b integer DEFAULT 2
  );
  ALTER TABLE public.gtest2 OWNER TO dgustafsson;
  CREATE TABLE public.gtest2_2 (
  )
  INHERITS (public.gtest2);
  ALTER TABLE public.gtest2_2 OWNER TO dgustafsson;

Is there a case when the ALTER TABLE ..  SET DEFAULT command is required to
recreate the scheman for non-local attrdefs?  If so, then Maasahiko-san's patch
is where this conversation should continue.

> (I suspect you are right that binary-upgrade is a special case,
> but wouldn't the existing pg_upgrade test catch that?  Also,
> do we need more test cases?)

binary-upgrade is a special case, but one which is already handled by setting
.separate via shouldPrintColumn for attrdefs.  This means that we don't need to
account for this in dumpAttrDef.

cheers ./daniel

Leaving a bread-crumb for anyone searching the archives in future:
I've forcibly unified this thread into the older one at

https://www.postgresql.org/message-id/flat/15830.1575468847%40sss.pgh.pa.us

            regards, tom lane