Re: ATTACH PARTITION seems to ignore column generation status - Mailing list pgsql-hackers

From Tom Lane
Subject Re: ATTACH PARTITION seems to ignore column generation status
Date
Msg-id 378312.1673388786@sss.pgh.pa.us
Whole thread Raw
In response to Re: ATTACH PARTITION seems to ignore column generation status  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: ATTACH PARTITION seems to ignore column generation status
List pgsql-hackers
I wrote:
> Amit Langote <amitlangote09@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)
PARTITIONBY 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.  So maybe we should deem this a supported
case and remove ATTACH PARTITION's insistence that the generation
expressions match ... which I think would be a good thing anyway,
because that check-for-same-reverse-compiled-expression business
is pretty cheesy in itself.  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.

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?

            regards, tom lane



pgsql-hackers by date:

Previous
From: sirisha chamarthi
Date:
Subject: Wasted Vacuum cycles when OldestXmin is not moving
Next
From: Jacob Champion
Date:
Subject: Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert