Subject: ATTACH PARTITION seems to ignore column generation status

From: Tom Lane <[email-masked]>
Date: Thu, 05 Jan 2023 13:53:19 -0500

This does not seem good:

regression=# create table pp (a int, b int) partition by range(a);
CREATE TABLE
regression=# create table cc (a int generated always as (b+1) stored, b int);
CREATE TABLE
regression=# alter table pp attach partition cc for values from ('1') to ('10'); 
ALTER TABLE
regression=# insert into pp values(1,100);
INSERT 0 1
regression=# table pp;
  a  |  b  
-----+-----
 101 | 100
(1 row)

I'm not sure to what extent it's sensible for partitions to have
GENERATED columns that don't match their parent; but even if that's
okay for payload columns I doubt we want to allow partitioning
columns to be GENERATED.

			regards, tom lane

---

From: Amit Langote <[email-masked]>
Date: Fri, 6 Jan 2023 17:26:58 +0900

> regression=# insert into pp values(1,100);

This indeed is broken and seems like an oversight. :-(

> okay for payload columns I doubt we want to allow partitioning

Actually, I'm inclined to disallow partitions to have *any* generated
columns that are not present in the parent table.  The main reason for
that is the inconvenience of checking that a partition's generated
columns doesn't override the partition key column of an ancestor that
is not its immediate parent, which MergeAttributesIntoExisting() has
access to and would have been locked.

Patch doing it that way is attached.  Perhaps the newly added error
message should match CREATE TABLE .. PARTITION OF's, but I found the
latter to be not detailed enough, or maybe that's just me.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

---

From: Tom Lane <[email-masked]>
Date: Fri, 06 Jan 2023 12:32:52 -0500

> access to and would have been locked.

After thinking about this awhile, I feel that we ought to disallow
it in the traditional-inheritance case as well.  The reason is that
there are semantic prohibitions on inserting or updating a generated
column, eg

regression=# create table t (f1 int, f2 int generated always as (f1+1) stored);
CREATE TABLE
regression=# update t set f2=42;
ERROR:  column "f2" can only be updated to DEFAULT
DETAIL:  Column "f2" is a generated column.

It's not very reasonable to have to recheck that for child tables,
and we don't.  But if one does this:

regression=# create table pp (f1 int, f2 int);
CREATE TABLE
regression=# create table cc (f1 int, f2 int generated always as (f1+1) stored) inherits(pp);
NOTICE:  merging column "f1" with inherited definition
NOTICE:  merging column "f2" with inherited definition
CREATE TABLE
regression=# insert into cc values(1);
INSERT 0 1
regression=# update pp set f2 = 99 where f1 = 1;
UPDATE 1
regression=# table cc;
 f1 | f2 
----+----
  1 | 99
(1 row)

That is surely just as broken as the partition-based case.

I also note that the code adjacent to what you added is

            /*
             * If parent column is generated, child column must be, too.
             */
            if (attribute->attgenerated && !childatt->attgenerated)
                ereport(ERROR, ...

without any exception for non-partition inheritance, and the
following check for equivalent generation expressions has
no such exception either.  So it's not very clear why this
test should have an exception.

> latter to be not detailed enough, or maybe that's just me.

Maybe we should improve the existing error message while we're at it?

			regards, tom lane