I have been analyzing this issue again. We have a few candidate patches
that do very similar things for avoiding dumping the generation
expression of table gtest1_1. We can figure out later which one of
these we like best. But there is another issue lurking nearby. The
table hierarchy of gtest30, which is created in the regression tests
like this:
CREATE TABLE gtest30 (
a int,
b int GENERATED ALWAYS AS (a * 2) STORED
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
ALTER TABLE ONLY gtest30 ALTER COLUMN b DROP EXPRESSION;
This drops the generation expression from the parent table but not the
child table. This is currently dumped like this:
CREATE TABLE public.gtest30 (
a integer,
b integer
);
CREATE TABLE public.gtest30_1 (
)
INHERITS (public.gtest30);
ALTER TABLE ONLY public.gtest30_1 ALTER COLUMN b SET DEFAULT (a * 2);
The proposed patches will cause the last statement to be omitted, but
that still won't recreate the original state. The problem is that there
is no command to make a column generated afterwards, like the SET
DEFAULT command, so we can't dump it like this. We would have to produce
CREATE TABLE public.gtest30 (
a integer,
b integer
);
CREATE TABLE public.gtest30_1 (
b integer GENERATED ALWAYS AS (a * 2) STORED
)
INHERITS (public.gtest30);
but this will create the column "b" of gtest30_1 as attlocal, which the
original command sequence does not.
We could probably fix this by having ALTER TABLE ONLY / DROP EXPRESSION
update the attlocal column of direct children to true, to make the
catalog state look like something that can be restored. However, that's
a fair amount of complicated code, so for now I propose to just prohibit
this command, meaning you can't use ONLY in this command if there are
children. This is new in PG13, so this change would have very limited
impact in practice.
Proposed patch attached.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 6c4209883dad527eb1140a61ed7ac6a610cf2a14 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Fri, 25 Sep 2020 14:51:59 +0200
Subject: [PATCH] Disallow ALTER TABLE ONLY / DROP EXPRESSION
The current implementation cannot handle this correct, so just forbid
it for now.
GENERATED clauses must be attached to the column definition and cannot
be added later like DEFAULT, so if a child table has a generation
expression that the parent does not have, the child column will
necessarily be an attlocal column. So to implement ALTER TABLE ONLY /
DROP EXPRESSION, we'd need extra code to update attislocal of the
direct child tables, somewhat similar to how DROP COLUMN does it, so
that the resulting state can be properly dumped and restored.
---
src/backend/commands/tablecmds.c | 22 +++++++++++++++++++---
src/test/regress/expected/generated.out | 11 ++++++-----
src/test/regress/sql/generated.sql | 2 +-
3 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 16285ad09f..b1b1b1e74a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -412,7 +412,7 @@ static ObjectAddress ATExecAddIdentity(Relation rel, const
char *colName,
static ObjectAddress ATExecSetIdentity(Relation rel, const char *colName,
Node
*def, LOCKMODE lockmode);
static ObjectAddress ATExecDropIdentity(Relation rel, const char *colName,
bool missing_ok, LOCKMODE lockmode);
-static void ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool
recursing);
+static void ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool
recurse, bool recursing, LOCKMODE lockmode);
static ObjectAddress ATExecDropExpression(Relation rel, const char *colName,
bool missing_ok, LOCKMODE lockmode);
static ObjectAddress ATExecSetStatistics(Relation rel, const char *colName,
int16 colNum,
Node *newValue, LOCKMODE lockmode);
@@ -4142,7 +4142,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_DropExpression: /* ALTER COLUMN DROP EXPRESSION */
ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode,
context);
- ATPrepDropExpression(rel, cmd, recursing);
+ ATPrepDropExpression(rel, cmd, recurse, recursing,
lockmode);
pass = AT_PASS_DROP;
break;
case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */
@@ -7240,8 +7240,24 @@ ATExecDropIdentity(Relation rel, const char *colName,
bool missing_ok, LOCKMODE
* ALTER TABLE ALTER COLUMN DROP EXPRESSION
*/
static void
-ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recursing)
+ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recurse, bool
recursing, LOCKMODE lockmode)
{
+ /*
+ * Reject ONLY if there are child tables. We could implement this, but
it
+ * is a bit complicated. GENERATED clauses must be attached to the
column
+ * definition and cannot be added later like DEFAULT, so if a child
table
+ * has a generation expression that the parent does not have, the child
+ * column will necessarily be an attlocal column. So to implement ONLY
+ * here, we'd need extra code to update attislocal of the direct child
+ * tables, somewhat similar to how DROP COLUMN does it, so that the
+ * resulting state can be properly dumped and restored.
+ */
+ if (!recurse &&
+ find_inheritance_children(RelationGetRelid(rel), lockmode))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ALTER TABLE / DROP EXPRESSION must be
applied to child tables too")));
+
/*
* Cannot drop generation expression from inherited columns.
*/
diff --git a/src/test/regress/expected/generated.out
b/src/test/regress/expected/generated.out
index 7ccc3c65ed..4b06260304 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -805,13 +805,14 @@ CREATE TABLE gtest30 (
b int GENERATED ALWAYS AS (a * 2) STORED
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
-ALTER TABLE ONLY gtest30 ALTER COLUMN b DROP EXPRESSION;
+ALTER TABLE ONLY gtest30 ALTER COLUMN b DROP EXPRESSION; -- error
+ERROR: ALTER TABLE / DROP EXPRESSION must be applied to child tables too
\d gtest30
- Table "public.gtest30"
- Column | Type | Collation | Nullable | Default
---------+---------+-----------+----------+---------
+ Table "public.gtest30"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+------------------------------------
a | integer | | |
- b | integer | | |
+ b | integer | | | generated always as (a * 2) stored
Number of child tables: 1 (Use \d+ to list them.)
\d gtest30_1
diff --git a/src/test/regress/sql/generated.sql
b/src/test/regress/sql/generated.sql
index 4cff1279c7..c86ad34b00 100644
--- a/src/test/regress/sql/generated.sql
+++ b/src/test/regress/sql/generated.sql
@@ -411,7 +411,7 @@ CREATE TABLE gtest30 (
b int GENERATED ALWAYS AS (a * 2) STORED
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
-ALTER TABLE ONLY gtest30 ALTER COLUMN b DROP EXPRESSION;
+ALTER TABLE ONLY gtest30 ALTER COLUMN b DROP EXPRESSION; -- error
\d gtest30
\d gtest30_1
ALTER TABLE gtest30_1 ALTER COLUMN b DROP EXPRESSION; -- error
--
2.28.0
