Re: ATTACH PARTITION seems to ignore column generation status

On Wed, Jan 11, 2023 at 7:13 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
> > Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> >> Thanks for the patch. It looks good, though I guess you said that we
> >> should also change the error message that CREATE TABLE ... PARTITION
> >> OF emits to match the other cases while we're here. I've attached a
> >> delta patch.
>
> > Thanks. I hadn't touched that issue because I wasn't entirely sure
> > which error message(s) you were unhappy with. These changes look
> > fine offhand.
>
> So, after playing with that a bit ... removing the block in
> parse_utilcmd.c allows you to do
>
> regression=# CREATE TABLE gtest_parent (f1 date NOT NULL, f2 bigint, f3 bigint GENERATED ALWAYS AS (f2 * 2) STORED) PARTITION BY RANGE (f1);
> CREATE TABLE
> regression=# CREATE TABLE gtest_child PARTITION OF gtest_parent (
> regression(# f3 WITH OPTIONS GENERATED ALWAYS AS (f2 * 3) STORED
> regression(# ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
> CREATE TABLE
> regression=# \d gtest_child
> Table "public.gtest_child"
> Column | Type | Collation | Nullable | Default
> --------+--------+-----------+----------+-------------------------------------
> f1 | date | | not null |
> f2 | bigint | | |
> f3 | bigint | | | generated always as (f2 * 3) stored
> Partition of: gtest_parent FOR VALUES FROM ('2016-07-01') TO ('2016-08-01')
>
> regression=# insert into gtest_parent values('2016-07-01', 42);
> INSERT 0 1
> regression=# table gtest_parent;
> f1 | f2 | f3
> ------------+----+-----
> 2016-07-01 | 42 | 126
> (1 row)
>
> That is, you can make a partition with a different generated expression
> than the parent has. Do we really want to allow that? I think it works
> as far as the backend is concerned, but it breaks pg_dump, which tries
> to dump this state of affairs as
>
> CREATE TABLE public.gtest_parent (
> f1 date NOT NULL,
> f2 bigint,
> f3 bigint GENERATED ALWAYS AS ((f2 * 2)) STORED
> )
> PARTITION BY RANGE (f1);
>
> CREATE TABLE public.gtest_child (
> f1 date NOT NULL,
> f2 bigint,
> f3 bigint GENERATED ALWAYS AS ((f2 * 3)) STORED
> );
>
> ALTER TABLE ONLY public.gtest_parent ATTACH PARTITION public.gtest_child FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
>
> and that fails at reload because the ATTACH PARTITION code path
> checks for equivalence of the generation expressions.
>
> This different-generated-expression situation isn't really morally
> different from different ordinary DEFAULT expressions, which we
> do endeavor to support.

Ah, right, we are a bit more flexible in allowing that. Though,
partition-specific defaults, unlike generated columns, are not
respected when inserting/updating via the parent:

create table partp (a int, b int generated always as (a+1) stored, c
int default 0) partition by list (a);
create table partc1 partition of partp (b generated always as (a+2)
stored, c default 1) for values in (1);
insert into partp values (1);
table partp;
a | b | c
---+---+---
1 | 3 | 0
(1 row)

create table partc2 partition of partp (b generated always as (a+2)
stored) for values in (2);
update partp set a = 2;
table partp;
a | b | c
---+---+---
2 | 4 | 0
(1 row)

> So maybe we should deem this a supported
> case and remove ATTACH PARTITION's insistence that the generation
> expressions match

I tend to agree now that we have 3f7836ff6.

> ... which I think would be a good thing anyway,
> because that check-for-same-reverse-compiled-expression business
> is pretty cheesy in itself.

Hmm, yeah, we usually transpose a parent's expression into one that
has a child's attribute numbers and vice versa when checking their
equivalence.

> AFAIK, 3f7836ff6 took care of the
> only problem that the backend would have with this, and pg_dump
> looks like it will work as long as the backend will take the
> ATTACH command.

Right.

> I also looked into making CREATE TABLE ... PARTITION OF reject
> this case; but that's much harder than it sounds, because what we
> have at the relevant point is a raw (unanalyzed) expression for
> the child's generation expression but a cooked one for the
> parent's, so there is no easy way to match the two.
>
> In short, it's seeming like the rule for both partitioning and
> traditional inheritance ought to be "a child column must have
> the same generated-or-not property as its parent, but their
> generation expressions need not be the same". Thoughts?

Agreed.

I've updated your disallow-generated-child-columns-2.patch to do this,
and have also merged the delta post that I had attached with my last
email, whose contents you sound to agree with.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com