Re: BUG #16622: pg_dump produces erroneus ALTER TABLE statement for a table with an inherited generated column

> On 21 Sep 2020, at 23:20, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
>>> On 18 Sep 2020, at 06:58, PG Bug reporting form <noreply(at)postgresql(dot)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