pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedATTACH PARTITION seems to ignore column generation status
14+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2023-02-16 a0fa18cc0d Fix check for child column generation status matching parent.
2023-01-12 d93d68aeea Doc: fix silly thinko in 8bf6ec3ba.
2023-01-11 8bf6ec3ba3 Improve handling of inherited GENERATED expressions.
2023-01-05 3f7836ff65 ad38e2f891 3706cc97aa 8cd190e13a Fix calculation of which GENERATED columns need to be updated.
* ATTACH PARTITION seems to ignore column generation status
@ 2023-01-05 18:53 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 14+ messages in thread
From: Tom Lane @ 2023-01-05 18:53 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: ATTACH PARTITION seems to ignore column generation status
@ 2023-01-06 08:26 Amit Langote <amitlangote09@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 14+ messages in thread
From: Amit Langote @ 2023-01-06 08:26 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org
On Fri, Jan 6, 2023 at 3:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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)
This indeed is broken and seems like an oversight. :-(
> 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.
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
Attachments:
disallow-partition-only-generated-cols.patch (application/octet-stream, 2.9K) download
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1db3bd9e2e..b3600b2dbd 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15046,6 +15046,18 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
errmsg("column \"%s\" in child table must be a generated column",
attributeName)));
+ /*
+ * Disallow partitions to have generated columns that are not in
+ * the parent table too.
+ */
+ if (child_is_partition &&
+ !attribute->attgenerated && childatt->attgenerated)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("column \"%s\" is not a generated column in parent table",
+ attributeName),
+ errdetail("Partitions cannot have generated columns that are not present in the parent table.")));
+
/*
* Check that both generation expressions match.
*
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index 1db5f9ed47..28f648bb5e 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -702,7 +702,11 @@ CREATE TABLE gtest_child PARTITION OF gtest_parent (
f3 WITH OPTIONS GENERATED ALWAYS AS (f2 * 2) STORED
) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
ERROR: generated columns are not supported on partitions
-DROP TABLE gtest_parent;
+CREATE TABLE gtest_child (f1 date NOT NULL, f2 text, f3 bigint GENERATED ALWAYS AS (2 * 2) STORED);
+ALTER TABLE gtest_parent ATTACH PARTITION gtest_child FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
+ERROR: column "f3" is not a generated column in parent table
+DETAIL: Partitions cannot have generated columns that are not present in the parent table.
+DROP TABLE gtest_parent, gtest_child;
-- partitioned table
CREATE TABLE gtest_parent (f1 date NOT NULL, f2 bigint, f3 bigint GENERATED ALWAYS AS (f2 * 2) STORED) PARTITION BY RANGE (f1);
CREATE TABLE gtest_child PARTITION OF gtest_parent FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/generated.sql
index 39eec40bce..2962181dc2 100644
--- a/src/test/regress/sql/generated.sql
+++ b/src/test/regress/sql/generated.sql
@@ -370,7 +370,9 @@ CREATE TABLE gtest_parent (f1 date NOT NULL, f2 text, f3 bigint) PARTITION BY RA
CREATE TABLE gtest_child PARTITION OF gtest_parent (
f3 WITH OPTIONS GENERATED ALWAYS AS (f2 * 2) STORED
) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
-DROP TABLE gtest_parent;
+CREATE TABLE gtest_child (f1 date NOT NULL, f2 text, f3 bigint GENERATED ALWAYS AS (2 * 2) STORED);
+ALTER TABLE gtest_parent ATTACH PARTITION gtest_child FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
+DROP TABLE gtest_parent, gtest_child;
-- partitioned table
CREATE TABLE gtest_parent (f1 date NOT NULL, f2 bigint, f3 bigint GENERATED ALWAYS AS (f2 * 2) STORED) PARTITION BY RANGE (f1);
* Re: ATTACH PARTITION seems to ignore column generation status
@ 2023-01-06 17:32 Tom Lane <tgl@sss.pgh.pa.us>
parent: Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Tom Lane @ 2023-01-06 17:32 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> writes:
> On Fri, Jan 6, 2023 at 3:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.
> 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.
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.
> 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.
Maybe we should improve the existing error message while we're at it?
regards, tom lane
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: ATTACH PARTITION seems to ignore column generation status
@ 2023-01-09 21:41 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 14+ messages in thread
From: Tom Lane @ 2023-01-09 21:41 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
I wrote:
> 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.
So what we need is about like this. This is definitely not something
to back-patch, since it's taking away what had been a documented
behavior. You could imagine trying to prevent such UPDATEs instead,
but I judge it not worth the trouble. If anyone were actually using
this capability we'd have heard bug reports.
regards, tom lane
Attachments:
disallow-generated-child-columns-2.patch (text/x-diff, 7.7K) download
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index d91a781479..6b60cd80ae 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -344,8 +344,8 @@ CREATE TABLE people (
</listitem>
<listitem>
<para>
- If a parent column is not a generated column, a child column may be
- defined to be a generated column or not.
+ If a parent column is not a generated column, a child column must
+ not be generated either.
</para>
</listitem>
</itemizedlist>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1db3bd9e2e..72ad6507d7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2931,6 +2931,11 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
* also check that the child column doesn't specify a default
* value or identity, which matches the rules for a single
* column in parse_util.c.
+ *
+ * Conversely, if the parent column is not generated, the
+ * child column can't be either. (We used to allow that, but
+ * it results in being able to override the generation
+ * expression via UPDATEs through the parent.)
*/
if (def->generated)
{
@@ -2951,15 +2956,14 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
errmsg("column \"%s\" inherits from generated column but specifies identity",
def->colname)));
}
-
- /*
- * If the parent column is not generated, then take whatever
- * the child column definition says.
- */
else
{
if (newdef->generated)
- def->generated = newdef->generated;
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
+ errmsg("child column \"%s\" specifies generation expression",
+ def->colname),
+ errhint("A child table column cannot be generated unless its parent column is.")));
}
/* If new def has a default, override previous default */
@@ -15038,13 +15042,18 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
attributeName)));
/*
- * If parent column is generated, child column must be, too.
+ * Child column must be generated if and only if parent column is.
*/
if (attribute->attgenerated && !childatt->attgenerated)
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("column \"%s\" in child table must be a generated column",
attributeName)));
+ if (childatt->attgenerated && !attribute->attgenerated)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("column \"%s\" in child table must not be a generated column",
+ attributeName)));
/*
* Check that both generation expressions match.
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index 1db5f9ed47..3c10dabf6d 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -268,38 +268,17 @@ SELECT * FROM gtest1;
4 | 8
(2 rows)
+-- can't have generated column that is a child of normal column
CREATE TABLE gtest_normal (a int, b int);
-CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2) STORED) INHERITS (gtest_normal);
+CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2) STORED) INHERITS (gtest_normal); -- error
NOTICE: merging column "a" with inherited definition
NOTICE: merging column "b" with inherited definition
-\d gtest_normal_child
- Table "public.gtest_normal_child"
- Column | Type | Collation | Nullable | Default
---------+---------+-----------+----------+------------------------------------
- a | integer | | |
- b | integer | | | generated always as (a * 2) stored
-Inherits: gtest_normal
-
-INSERT INTO gtest_normal (a) VALUES (1);
-INSERT INTO gtest_normal_child (a) VALUES (2);
-SELECT * FROM gtest_normal;
- a | b
----+---
- 1 |
- 2 | 4
-(2 rows)
-
-CREATE TABLE gtest_normal_child2 (a int, b int GENERATED ALWAYS AS (a * 3) STORED);
-ALTER TABLE gtest_normal_child2 INHERIT gtest_normal;
-INSERT INTO gtest_normal_child2 (a) VALUES (3);
-SELECT * FROM gtest_normal;
- a | b
----+---
- 1 |
- 2 | 4
- 3 | 9
-(3 rows)
-
+ERROR: child column "b" specifies generation expression
+HINT: A child table column cannot be generated unless its parent column is.
+CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
+ALTER TABLE gtest_normal_child INHERIT gtest_normal; -- error
+ERROR: column "b" in child table must not be a generated column
+DROP TABLE gtest_normal, gtest_normal_child;
-- test inheritance mismatches between parent and child
CREATE TABLE gtestx (x int, b int GENERATED ALWAYS AS (a * 22) STORED) INHERITS (gtest1); -- error
NOTICE: merging column "b" with inherited definition
@@ -702,7 +681,10 @@ CREATE TABLE gtest_child PARTITION OF gtest_parent (
f3 WITH OPTIONS GENERATED ALWAYS AS (f2 * 2) STORED
) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
ERROR: generated columns are not supported on partitions
-DROP TABLE gtest_parent;
+CREATE TABLE gtest_child (f1 date NOT NULL, f2 text, f3 bigint GENERATED ALWAYS AS (2 * 2) STORED);
+ALTER TABLE gtest_parent ATTACH PARTITION gtest_child FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
+ERROR: column "f3" in child table must not be a generated column
+DROP TABLE gtest_parent, gtest_child;
-- partitioned table
CREATE TABLE gtest_parent (f1 date NOT NULL, f2 bigint, f3 bigint GENERATED ALWAYS AS (f2 * 2) STORED) PARTITION BY RANGE (f1);
CREATE TABLE gtest_child PARTITION OF gtest_parent FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/generated.sql
index 39eec40bce..01fc4ed892 100644
--- a/src/test/regress/sql/generated.sql
+++ b/src/test/regress/sql/generated.sql
@@ -110,17 +110,12 @@ INSERT INTO gtest1_1 VALUES (4);
SELECT * FROM gtest1_1;
SELECT * FROM gtest1;
+-- can't have generated column that is a child of normal column
CREATE TABLE gtest_normal (a int, b int);
-CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2) STORED) INHERITS (gtest_normal);
-\d gtest_normal_child
-INSERT INTO gtest_normal (a) VALUES (1);
-INSERT INTO gtest_normal_child (a) VALUES (2);
-SELECT * FROM gtest_normal;
-
-CREATE TABLE gtest_normal_child2 (a int, b int GENERATED ALWAYS AS (a * 3) STORED);
-ALTER TABLE gtest_normal_child2 INHERIT gtest_normal;
-INSERT INTO gtest_normal_child2 (a) VALUES (3);
-SELECT * FROM gtest_normal;
+CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2) STORED) INHERITS (gtest_normal); -- error
+CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
+ALTER TABLE gtest_normal_child INHERIT gtest_normal; -- error
+DROP TABLE gtest_normal, gtest_normal_child;
-- test inheritance mismatches between parent and child
CREATE TABLE gtestx (x int, b int GENERATED ALWAYS AS (a * 22) STORED) INHERITS (gtest1); -- error
@@ -370,7 +365,9 @@ CREATE TABLE gtest_parent (f1 date NOT NULL, f2 text, f3 bigint) PARTITION BY RA
CREATE TABLE gtest_child PARTITION OF gtest_parent (
f3 WITH OPTIONS GENERATED ALWAYS AS (f2 * 2) STORED
) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
-DROP TABLE gtest_parent;
+CREATE TABLE gtest_child (f1 date NOT NULL, f2 text, f3 bigint GENERATED ALWAYS AS (2 * 2) STORED);
+ALTER TABLE gtest_parent ATTACH PARTITION gtest_child FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
+DROP TABLE gtest_parent, gtest_child;
-- partitioned table
CREATE TABLE gtest_parent (f1 date NOT NULL, f2 bigint, f3 bigint GENERATED ALWAYS AS (f2 * 2) STORED) PARTITION BY RANGE (f1);
* Re: ATTACH PARTITION seems to ignore column generation status
@ 2023-01-10 02:30 Amit Langote <amitlangote09@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 14+ messages in thread
From: Amit Langote @ 2023-01-10 02:30 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
On Tue, Jan 10, 2023 at 6:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > 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.
Agree that it looks bad.
> So what we need is about like this. This is definitely not something
> to back-patch, since it's taking away what had been a documented
> behavior. You could imagine trying to prevent such UPDATEs instead,
> but I judge it not worth the trouble. If anyone were actually using
> this capability we'd have heard bug reports.
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, Amit Langote
EDB: http://www.enterprisedb.com
Attachments:
partition-generated-cols-different-error.patch (application/octet-stream, 2.6K) download
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 72ad6507d7..100891861e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3013,11 +3013,22 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
{
ColumnDef *coldef = lfirst(l);
+ /*
+ * Like above, prevent generated columns in partitions that
+ * are not present in the parent.
+ */
if (strcmp(coldef->colname, restdef->colname) == 0)
{
found = true;
coldef->is_not_null |= restdef->is_not_null;
+ if (restdef->generated && !coldef->generated)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
+ errmsg("child column \"%s\" specifies generation expression",
+ restdef->colname),
+ errhint("A child table column cannot be generated unless its parent column is.")));
+
/*
* Override the parent's default value for this column
* (coldef->cooked_default) with the partition's local
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index bffa9f8dd0..f9218f48aa 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -740,11 +740,6 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("generated columns are not supported on typed tables")));
- if (cxt->partbound)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("generated columns are not supported on partitions")));
-
if (saw_generated)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index 3c10dabf6d..4a7b014982 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -680,7 +680,8 @@ CREATE TABLE gtest_parent (f1 date NOT NULL, f2 text, f3 bigint) PARTITION BY RA
CREATE TABLE gtest_child PARTITION OF gtest_parent (
f3 WITH OPTIONS GENERATED ALWAYS AS (f2 * 2) STORED
) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
-ERROR: generated columns are not supported on partitions
+ERROR: child column "f3" specifies generation expression
+HINT: A child table column cannot be generated unless its parent column is.
CREATE TABLE gtest_child (f1 date NOT NULL, f2 text, f3 bigint GENERATED ALWAYS AS (2 * 2) STORED);
ALTER TABLE gtest_parent ATTACH PARTITION gtest_child FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
ERROR: column "f3" in child table must not be a generated column
* Re: ATTACH PARTITION seems to ignore column generation status
@ 2023-01-10 02:38 Tom Lane <tgl@sss.pgh.pa.us>
parent: Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Tom Lane @ 2023-01-10 02:38 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
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.
regards, tom lane
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: ATTACH PARTITION seems to ignore column generation status
@ 2023-01-10 22:13 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 14+ messages in thread
From: Tom Lane @ 2023-01-10 22:13 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
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) 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. 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
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: ATTACH PARTITION seems to ignore column generation status
@ 2023-01-11 03:43 Amit Langote <amitlangote09@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 14+ messages in thread
From: Amit Langote @ 2023-01-11 03:43 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
On Wed, Jan 11, 2023 at 7:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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) 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
Attachments:
disallow-generated-child-columns-3.patch (application/octet-stream, 13.0K) download
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 4b219435d4..780713831c 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -330,9 +330,9 @@ CREATE TABLE people (
<listitem>
<para>
If a parent column is a generated column, a child column must also be
- a generated column using the same expression. In the definition of
- the child column, leave off the <literal>GENERATED</literal> clause,
- as it will be copied from the parent.
+ a generated column, with or without the same expression. In the
+ definition of the child column, leave off the <literal>GENERATED</literal>
+ clause, as it will be copied from the parent.
</para>
</listitem>
<listitem>
@@ -344,8 +344,8 @@ CREATE TABLE people (
</listitem>
<listitem>
<para>
- If a parent column is not a generated column, a child column may be
- defined to be a generated column or not.
+ If a parent column is not a generated column, a child column must
+ not be generated either.
</para>
</listitem>
</itemizedlist>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1db3bd9e2e..6ef23d138d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2931,6 +2931,11 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
* also check that the child column doesn't specify a default
* value or identity, which matches the rules for a single
* column in parse_util.c.
+ *
+ * Conversely, if the parent column is not generated, the
+ * child column can't be either. (We used to allow that, but
+ * it results in being able to override the generation
+ * expression via UPDATEs through the parent.)
*/
if (def->generated)
{
@@ -2951,15 +2956,14 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
errmsg("column \"%s\" inherits from generated column but specifies identity",
def->colname)));
}
-
- /*
- * If the parent column is not generated, then take whatever
- * the child column definition says.
- */
else
{
if (newdef->generated)
- def->generated = newdef->generated;
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
+ errmsg("child column \"%s\" specifies generation expression",
+ def->colname),
+ errhint("A child table column cannot be generated unless its parent column is.")));
}
/* If new def has a default, override previous default */
@@ -3009,11 +3013,22 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
{
ColumnDef *coldef = lfirst(l);
+ /*
+ * Like above, prevent generated columns in partitions that
+ * are not present in the parent.
+ */
if (strcmp(coldef->colname, restdef->colname) == 0)
{
found = true;
coldef->is_not_null |= restdef->is_not_null;
+ if (restdef->generated && !coldef->generated)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
+ errmsg("child column \"%s\" specifies generation expression",
+ restdef->colname),
+ errhint("A child table column cannot be generated unless its parent column is.")));
+
/*
* Override the parent's default value for this column
* (coldef->cooked_default) with the partition's local
@@ -15038,64 +15053,18 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
attributeName)));
/*
- * If parent column is generated, child column must be, too.
+ * Child column must be generated if and only if parent column is.
*/
if (attribute->attgenerated && !childatt->attgenerated)
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("column \"%s\" in child table must be a generated column",
attributeName)));
-
- /*
- * Check that both generation expressions match.
- *
- * The test we apply is to see whether they reverse-compile to the
- * same source string. This insulates us from issues like whether
- * attributes have the same physical column numbers in parent and
- * child relations. (See also constraints_equivalent().)
- */
- if (attribute->attgenerated && childatt->attgenerated)
- {
- TupleConstr *child_constr = child_rel->rd_att->constr;
- TupleConstr *parent_constr = parent_rel->rd_att->constr;
- char *child_expr = NULL;
- char *parent_expr = NULL;
-
- Assert(child_constr != NULL);
- Assert(parent_constr != NULL);
-
- for (int i = 0; i < child_constr->num_defval; i++)
- {
- if (child_constr->defval[i].adnum == childatt->attnum)
- {
- child_expr =
- TextDatumGetCString(DirectFunctionCall2(pg_get_expr,
- CStringGetTextDatum(child_constr->defval[i].adbin),
- ObjectIdGetDatum(child_rel->rd_id)));
- break;
- }
- }
- Assert(child_expr != NULL);
-
- for (int i = 0; i < parent_constr->num_defval; i++)
- {
- if (parent_constr->defval[i].adnum == attribute->attnum)
- {
- parent_expr =
- TextDatumGetCString(DirectFunctionCall2(pg_get_expr,
- CStringGetTextDatum(parent_constr->defval[i].adbin),
- ObjectIdGetDatum(parent_rel->rd_id)));
- break;
- }
- }
- Assert(parent_expr != NULL);
-
- if (strcmp(child_expr, parent_expr) != 0)
- ereport(ERROR,
- (errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("column \"%s\" in child table has a conflicting generation expression",
- attributeName)));
- }
+ if (childatt->attgenerated && !attribute->attgenerated)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("column \"%s\" in child table must not be a generated column",
+ attributeName)));
/*
* OK, bump the child column's inheritance count. (If we fail
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index bffa9f8dd0..f9218f48aa 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -740,11 +740,6 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("generated columns are not supported on typed tables")));
- if (cxt->partbound)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("generated columns are not supported on partitions")));
-
if (saw_generated)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index 1db5f9ed47..585f3d482c 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -268,38 +268,17 @@ SELECT * FROM gtest1;
4 | 8
(2 rows)
+-- can't have generated column that is a child of normal column
CREATE TABLE gtest_normal (a int, b int);
-CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2) STORED) INHERITS (gtest_normal);
+CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2) STORED) INHERITS (gtest_normal); -- error
NOTICE: merging column "a" with inherited definition
NOTICE: merging column "b" with inherited definition
-\d gtest_normal_child
- Table "public.gtest_normal_child"
- Column | Type | Collation | Nullable | Default
---------+---------+-----------+----------+------------------------------------
- a | integer | | |
- b | integer | | | generated always as (a * 2) stored
-Inherits: gtest_normal
-
-INSERT INTO gtest_normal (a) VALUES (1);
-INSERT INTO gtest_normal_child (a) VALUES (2);
-SELECT * FROM gtest_normal;
- a | b
----+---
- 1 |
- 2 | 4
-(2 rows)
-
-CREATE TABLE gtest_normal_child2 (a int, b int GENERATED ALWAYS AS (a * 3) STORED);
-ALTER TABLE gtest_normal_child2 INHERIT gtest_normal;
-INSERT INTO gtest_normal_child2 (a) VALUES (3);
-SELECT * FROM gtest_normal;
- a | b
----+---
- 1 |
- 2 | 4
- 3 | 9
-(3 rows)
-
+ERROR: child column "b" specifies generation expression
+HINT: A child table column cannot be generated unless its parent column is.
+CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
+ALTER TABLE gtest_normal_child INHERIT gtest_normal; -- error
+ERROR: column "b" in child table must not be a generated column
+DROP TABLE gtest_normal, gtest_normal_child;
-- test inheritance mismatches between parent and child
CREATE TABLE gtestx (x int, b int GENERATED ALWAYS AS (a * 22) STORED) INHERITS (gtest1); -- error
NOTICE: merging column "b" with inherited definition
@@ -314,9 +293,6 @@ ERROR: column "b" inherits from generated column but specifies identity
CREATE TABLE gtestxx_1 (a int NOT NULL, b int);
ALTER TABLE gtestxx_1 INHERIT gtest1; -- error
ERROR: column "b" in child table must be a generated column
-CREATE TABLE gtestxx_2 (a int NOT NULL, b int GENERATED ALWAYS AS (a * 22) STORED);
-ALTER TABLE gtestxx_2 INHERIT gtest1; -- error
-ERROR: column "b" in child table has a conflicting generation expression
CREATE TABLE gtestxx_3 (a int NOT NULL, b int GENERATED ALWAYS AS (a * 2) STORED);
ALTER TABLE gtestxx_3 INHERIT gtest1; -- ok
CREATE TABLE gtestxx_4 (b int GENERATED ALWAYS AS (a * 2) STORED, a int NOT NULL);
@@ -701,8 +677,12 @@ CREATE TABLE gtest_parent (f1 date NOT NULL, f2 text, f3 bigint) PARTITION BY RA
CREATE TABLE gtest_child PARTITION OF gtest_parent (
f3 WITH OPTIONS GENERATED ALWAYS AS (f2 * 2) STORED
) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
-ERROR: generated columns are not supported on partitions
-DROP TABLE gtest_parent;
+ERROR: child column "f3" specifies generation expression
+HINT: A child table column cannot be generated unless its parent column is.
+CREATE TABLE gtest_child (f1 date NOT NULL, f2 text, f3 bigint GENERATED ALWAYS AS (2 * 2) STORED);
+ALTER TABLE gtest_parent ATTACH PARTITION gtest_child FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
+ERROR: column "f3" in child table must not be a generated column
+DROP TABLE gtest_parent, gtest_child;
-- partitioned table
CREATE TABLE gtest_parent (f1 date NOT NULL, f2 bigint, f3 bigint GENERATED ALWAYS AS (f2 * 2) STORED) PARTITION BY RANGE (f1);
CREATE TABLE gtest_child PARTITION OF gtest_parent FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/generated.sql
index 39eec40bce..c962b1b226 100644
--- a/src/test/regress/sql/generated.sql
+++ b/src/test/regress/sql/generated.sql
@@ -110,17 +110,12 @@ INSERT INTO gtest1_1 VALUES (4);
SELECT * FROM gtest1_1;
SELECT * FROM gtest1;
+-- can't have generated column that is a child of normal column
CREATE TABLE gtest_normal (a int, b int);
-CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2) STORED) INHERITS (gtest_normal);
-\d gtest_normal_child
-INSERT INTO gtest_normal (a) VALUES (1);
-INSERT INTO gtest_normal_child (a) VALUES (2);
-SELECT * FROM gtest_normal;
-
-CREATE TABLE gtest_normal_child2 (a int, b int GENERATED ALWAYS AS (a * 3) STORED);
-ALTER TABLE gtest_normal_child2 INHERIT gtest_normal;
-INSERT INTO gtest_normal_child2 (a) VALUES (3);
-SELECT * FROM gtest_normal;
+CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2) STORED) INHERITS (gtest_normal); -- error
+CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
+ALTER TABLE gtest_normal_child INHERIT gtest_normal; -- error
+DROP TABLE gtest_normal, gtest_normal_child;
-- test inheritance mismatches between parent and child
CREATE TABLE gtestx (x int, b int GENERATED ALWAYS AS (a * 22) STORED) INHERITS (gtest1); -- error
@@ -129,8 +124,6 @@ CREATE TABLE gtestx (x int, b int GENERATED ALWAYS AS IDENTITY) INHERITS (gtest1
CREATE TABLE gtestxx_1 (a int NOT NULL, b int);
ALTER TABLE gtestxx_1 INHERIT gtest1; -- error
-CREATE TABLE gtestxx_2 (a int NOT NULL, b int GENERATED ALWAYS AS (a * 22) STORED);
-ALTER TABLE gtestxx_2 INHERIT gtest1; -- error
CREATE TABLE gtestxx_3 (a int NOT NULL, b int GENERATED ALWAYS AS (a * 2) STORED);
ALTER TABLE gtestxx_3 INHERIT gtest1; -- ok
CREATE TABLE gtestxx_4 (b int GENERATED ALWAYS AS (a * 2) STORED, a int NOT NULL);
@@ -370,7 +363,9 @@ CREATE TABLE gtest_parent (f1 date NOT NULL, f2 text, f3 bigint) PARTITION BY RA
CREATE TABLE gtest_child PARTITION OF gtest_parent (
f3 WITH OPTIONS GENERATED ALWAYS AS (f2 * 2) STORED
) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
-DROP TABLE gtest_parent;
+CREATE TABLE gtest_child (f1 date NOT NULL, f2 text, f3 bigint GENERATED ALWAYS AS (2 * 2) STORED);
+ALTER TABLE gtest_parent ATTACH PARTITION gtest_child FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
+DROP TABLE gtest_parent, gtest_child;
-- partitioned table
CREATE TABLE gtest_parent (f1 date NOT NULL, f2 bigint, f3 bigint GENERATED ALWAYS AS (f2 * 2) STORED) PARTITION BY RANGE (f1);
* Re: ATTACH PARTITION seems to ignore column generation status
@ 2023-01-11 20:58 Tom Lane <tgl@sss.pgh.pa.us>
parent: Amit Langote <amitlangote09@gmail.com>
0 siblings, 2 replies; 14+ messages in thread
From: Tom Lane @ 2023-01-11 20:58 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Amit Langote <amitlangote09@gmail.com> writes:
> 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.
Pushed with some further work to improve the handling of multiple-
inheritance cases. We still need to insist that all or none of the
parent columns are generated, but we don't have to require their
generation expressions to be alike: that can be resolved by letting
the child table override the expression, much as we've long done for
plain default expressions. (This did need some work in pg_dump
after all.) I'm pretty happy with where this turned out.
regards, tom lane
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: ATTACH PARTITION seems to ignore column generation status
@ 2023-01-12 03:00 Amit Langote <amitlangote09@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 14+ messages in thread
From: Amit Langote @ 2023-01-12 03:00 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
On Thu, Jan 12, 2023 at 5:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > 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.
>
> Pushed with some further work to improve the handling of multiple-
> inheritance cases. We still need to insist that all or none of the
> parent columns are generated, but we don't have to require their
> generation expressions to be alike: that can be resolved by letting
> the child table override the expression, much as we've long done for
> plain default expressions. (This did need some work in pg_dump
> after all.) I'm pretty happy with where this turned out.
Thanks, that all looks more consistent now indeed.
I noticed a typo in the doc additions, which I've attached a fix for.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Attachments:
8bf6ec3ba3-doc-typo.patch (application/octet-stream, 732B) download
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 1c0700d347..db40c47077 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -361,7 +361,7 @@ CREATE TABLE people (
definition without any <literal>GENERATED</literal> clause
in <command>CREATE TABLE ... PARTITION OF</command>, then
its <literal>GENERATED</literal> clause will automatically be copied
- from the parent. <command>ALTER TABLE ... ADD PARTITION</command>
+ from the parent. <command>ALTER TABLE ... ATTACH PARTITION</command>
will insist that parent and child columns already match as to
generation status, but it will not require their generation
expressions to match.
* Re: ATTACH PARTITION seems to ignore column generation status
@ 2023-01-12 03:20 Tom Lane <tgl@sss.pgh.pa.us>
parent: Amit Langote <amitlangote09@gmail.com>
0 siblings, 0 replies; 14+ messages in thread
From: Tom Lane @ 2023-01-12 03:20 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Amit Langote <amitlangote09@gmail.com> writes:
> I noticed a typo in the doc additions, which I've attached a fix for.
Doh, right, pushed.
regards, tom lane
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: ATTACH PARTITION seems to ignore column generation status
@ 2023-02-16 18:00 Alexander Lakhin <exclusion@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 14+ messages in thread
From: Alexander Lakhin @ 2023-02-16 18:00 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Amit Langote <amitlangote09@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Hello,
11.01.2023 23:58, Tom Lane wrote:
> Amit Langote<amitlangote09@gmail.com> writes:
>> 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.
> Pushed with some further work to improve the handling of multiple-
> inheritance cases. We still need to insist that all or none of the
> parent columns are generated, but we don't have to require their
> generation expressions to be alike: that can be resolved by letting
> the child table override the expression, much as we've long done for
> plain default expressions. (This did need some work in pg_dump
> after all.) I'm pretty happy with where this turned out.
I've encountered a query that triggers an assert added in that commit:
CREATE TABLE t(a int, b int GENERATED ALWAYS AS (a) STORED) PARTITION BY
RANGE (a);
CREATE TABLE tp PARTITION OF t(b DEFAULT 1) FOR VALUES FROM (0) to (1);
Core was generated by `postgres: law regression [local] CREATE
TABLE '.
Program terminated with signal SIGABRT, Aborted.
warning: Section `.reg-xstate/3152655' in core file too small.
#0 __pthread_kill_implementation (no_tid=0, signo=6,
threadid=140460372887360) at ./nptl/pthread_kill.c:44
44 ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0 __pthread_kill_implementation (no_tid=0, signo=6,
threadid=140460372887360) at ./nptl/pthread_kill.c:44
#1 __pthread_kill_internal (signo=6, threadid=140460372887360) at
./nptl/pthread_kill.c:78
#2 __GI___pthread_kill (threadid=140460372887360, signo=signo@entry=6)
at ./nptl/pthread_kill.c:89
#3 0x00007fbf79f0e476 in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#4 0x00007fbf79ef47f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x000055e76b35b322 in ExceptionalCondition (
conditionName=conditionName@entry=0x55e76b4a2240
"!(coldef->generated && !restdef->generated)",
fileName=fileName@entry=0x55e76b49ec71 "tablecmds.c",
lineNumber=lineNumber@entry=3028) at assert.c:66
#6 0x000055e76afef8c3 in MergeAttributes (schema=0x55e76d480318,
supers=supers@entry=0x55e76d474c18,
relpersistence=112 'p', is_partition=true,
supconstr=supconstr@entry=0x7ffe945a3768) at tablecmds.c:3028
#7 0x000055e76aff0ef2 in DefineRelation
(stmt=stmt@entry=0x55e76d44b2d8, relkind=relkind@entry=114 'r', ownerId=10,
ownerId@entry=0, typaddress=typaddress@entry=0x0,
queryString=queryString@entry=0x55e76d44a408 "CREATE TABLE tp
PARTITION OF t(b DEFAULT 1) FOR VALUES FROM (0) to (1);") at tablecmds.c:861
...
Without asserts enables, the partition created successfully, and
INSERT INTO t VALUES(0);
SELECT * FROM t;
yields:
a | b
---+---
0 | 1
(1 row)
Is this behavior expected?
Best regards,
Alexander
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: ATTACH PARTITION seems to ignore column generation status
@ 2023-02-16 18:05 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Alvaro Herrera @ 2023-02-16 18:05 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Langote <amitlangote09@gmail.com>; pgsql-hackers@lists.postgresql.org, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
On 2023-Feb-16, Alexander Lakhin wrote:
> I've encountered a query that triggers an assert added in that commit:
> CREATE TABLE t(a int, b int GENERATED ALWAYS AS (a) STORED) PARTITION BY
> RANGE (a);
> CREATE TABLE tp PARTITION OF t(b DEFAULT 1) FOR VALUES FROM (0) to (1);
It seems wrong that this command is accepted. It should have given an
error, because the partition is not allowed to override the generation
of the value that is specified in the parent table.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: ATTACH PARTITION seems to ignore column generation status
@ 2023-02-16 22:40 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 0 replies; 14+ messages in thread
From: Tom Lane @ 2023-02-16 22:40 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Alexander Lakhin <exclusion@gmail.com>; Amit Langote <amitlangote09@gmail.com>; pgsql-hackers@lists.postgresql.org, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2023-Feb-16, Alexander Lakhin wrote:
>> I've encountered a query that triggers an assert added in that commit:
>> CREATE TABLE t(a int, b int GENERATED ALWAYS AS (a) STORED) PARTITION BY
>> RANGE (a);
>> CREATE TABLE tp PARTITION OF t(b DEFAULT 1) FOR VALUES FROM (0) to (1);
> It seems wrong that this command is accepted. It should have given an
> error, because the partition is not allowed to override the generation
> of the value that is specified in the parent table.
Agreed. We missed a check somewhere, will fix.
regards, tom lane
^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2023-02-16 22:40 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-01-05 18:53 ATTACH PARTITION seems to ignore column generation status Tom Lane <tgl@sss.pgh.pa.us>
2023-01-06 08:26 ` Amit Langote <amitlangote09@gmail.com>
2023-01-06 17:32 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-01-09 21:41 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-01-10 02:30 ` Amit Langote <amitlangote09@gmail.com>
2023-01-10 02:38 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-01-10 22:13 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-01-11 03:43 ` Amit Langote <amitlangote09@gmail.com>
2023-01-11 20:58 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-01-12 03:00 ` Amit Langote <amitlangote09@gmail.com>
2023-01-12 03:20 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-02-16 18:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-02-16 18:05 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-16 22:40 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox