On Wed, Jan 11, 2023 at 7:13 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> I wrote:
> > Amit Langote <amitlangot...@gmail.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
disallow-generated-child-columns-3.patch
Description: Binary data
