Disallow USING clause when altering type of generated column
A USING clause when altering the type of a generated column does not
make sense. It would write the output of the USING clause into the
converted column, which would violate the generation expression.
This patch adds a check to error out if this is specified.
There was a test for this, but that test errored out for a different
reason, so it was not effective.
discovered by Jian He at [0]/messages/by-id/CACJufxEGPYtFe79hbsMeOBOivfNnPRsw7Gjvk67m1x2MQggyiQ@mail.gmail.com
[0]: /messages/by-id/CACJufxEGPYtFe79hbsMeOBOivfNnPRsw7Gjvk67m1x2MQggyiQ@mail.gmail.com
/messages/by-id/CACJufxEGPYtFe79hbsMeOBOivfNnPRsw7Gjvk67m1x2MQggyiQ@mail.gmail.com
On Wed, 21 Aug 2024 08:17:45 +0200
Peter Eisentraut <peter@eisentraut.org> wrote:
A USING clause when altering the type of a generated column does not
make sense. It would write the output of the USING clause into the
converted column, which would violate the generation expression.This patch adds a check to error out if this is specified.
I’m afraid you forgot to attach the patch.
It seems for me that this fix is reasonable though.
Regards,
Yugo Nagata
There was a test for this, but that test errored out for a different
reason, so it was not effective.discovered by Jian He at [0]
[0]:
/messages/by-id/CACJufxEGPYtFe79hbsMeOBOivfNnPRsw7Gjvk67m1x2MQggyiQ@mail.gmail.com
--
Yugo Nagata <nagata@sraoss.co.jp>
On 21.08.24 09:14, Yugo Nagata wrote:
On Wed, 21 Aug 2024 08:17:45 +0200
Peter Eisentraut <peter@eisentraut.org> wrote:A USING clause when altering the type of a generated column does not
make sense. It would write the output of the USING clause into the
converted column, which would violate the generation expression.This patch adds a check to error out if this is specified.
I’m afraid you forgot to attach the patch.
It seems for me that this fix is reasonable though.
Thanks, here is the patch.
Attachments:
0001-Disallow-USING-clause-when-altering-type-of-generate.patchtext/plain; charset=UTF-8; name=0001-Disallow-USING-clause-when-altering-type-of-generate.patchDownload
From de331c245f2c1becb2d1c7c7bb34429a18fa85b6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 21 Aug 2024 08:03:08 +0200
Subject: [PATCH] Disallow USING clause when altering type of generated column
This does not make sense. It would write the output of the USING
clause into the converted column, which would violate the generation
expression. This adds a check to error out if this is specified.
There was a test for this, but that test errored out for a different
reason, so it was not effective.
---
src/backend/commands/tablecmds.c | 10 ++++++++++
src/test/regress/expected/generated.out | 3 ++-
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7a36db6af6d..ee089e393fc 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -12736,6 +12736,16 @@ ATPrepAlterColumnType(List **wqueue,
errmsg("cannot alter system column \"%s\"",
colName)));
+ /*
+ * Cannot specify USING when altering type of a generated column, because
+ * that would violate the generation expression.
+ */
+ if (attTup->attgenerated && def->cooked_default)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("cannot specify USING when altering type of generated column"),
+ errdetail("Column \"%s\" is a generated column.", colName)));
+
/*
* Don't alter inherited columns. At outer level, there had better not be
* any inherited definition; when recursing, we assume this was checked at
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index 44058db7c1d..499072e14ca 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -1026,7 +1026,8 @@ SELECT * FROM gtest27;
(2 rows)
ALTER TABLE gtest27 ALTER COLUMN x TYPE boolean USING x <> 0; -- error
-ERROR: generation expression for column "x" cannot be cast automatically to type boolean
+ERROR: cannot specify USING when altering type of generated column
+DETAIL: Column "x" is a generated column.
ALTER TABLE gtest27 ALTER COLUMN x DROP DEFAULT; -- error
ERROR: column "x" of relation "gtest27" is a generated column
HINT: Use ALTER TABLE ... ALTER COLUMN ... DROP EXPRESSION instead.
--
2.46.0
On Wed, Aug 21, 2024 at 4:57 PM Peter Eisentraut <peter@eisentraut.org> wrote:
+ /*
+ * Cannot specify USING when altering type of a generated column, because
+ * that would violate the generation expression.
+ */
+ if (attTup->attgenerated && def->cooked_default)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("cannot specify USING when altering type of generated column"),
+ errdetail("Column \"%s\" is a generated column.", colName)));
+errcode should be ERRCODE_FEATURE_NOT_SUPPORTED?
also
CREATE TABLE gtest27 (
a int,
b text collate "C",
x text GENERATED ALWAYS AS ( b || '_2') STORED
);
ALTER TABLE gtest27 ALTER COLUMN x TYPE int;
ERROR: column "x" cannot be cast automatically to type integer
HINT: You might need to specify "USING x::integer".
should we do something for the errhint, since this specific errhint is wrong?
On Thu, 22 Aug 2024 11:38:49 +0800
jian he <jian.universality@gmail.com> wrote:
On Wed, Aug 21, 2024 at 4:57 PM Peter Eisentraut <peter@eisentraut.org> wrote:
+ /* + * Cannot specify USING when altering type of a generated column, because + * that would violate the generation expression. + */ + if (attTup->attgenerated && def->cooked_default) + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot specify USING when altering type of generated column"), + errdetail("Column \"%s\" is a generated column.", colName))); +errcode should be ERRCODE_FEATURE_NOT_SUPPORTED?
Although ERRCODE_INVALID_TABLE_DEFINITION is used for en error on changing
type of inherited column, I guess that is because it prevents from breaking
consistency between inherited and inheriting tables as a result of the command.
In this sense, maybe, ERRCODE_INVALID_COLUMN_DEFINITION is proper here, because
this check is to prevent inconsistency between columns in a tuple.
also
CREATE TABLE gtest27 (
a int,
b text collate "C",
x text GENERATED ALWAYS AS ( b || '_2') STORED
);ALTER TABLE gtest27 ALTER COLUMN x TYPE int;
ERROR: column "x" cannot be cast automatically to type integer
HINT: You might need to specify "USING x::integer".should we do something for the errhint, since this specific errhint is wrong?
Yes. I think we don't have to output the hint message if we disallow USING
for generated columns. Or, it may be useful to allow only a simple cast
for the generated column instead of completely prohibiting USING.
Regards,
Yugo Nagata
--
Yugo Nagata <nagata@sraoss.co.jp>
On 22.08.24 08:15, Yugo Nagata wrote:
On Thu, 22 Aug 2024 11:38:49 +0800
jian he <jian.universality@gmail.com> wrote:On Wed, Aug 21, 2024 at 4:57 PM Peter Eisentraut <peter@eisentraut.org> wrote:
+ /* + * Cannot specify USING when altering type of a generated column, because + * that would violate the generation expression. + */ + if (attTup->attgenerated && def->cooked_default) + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot specify USING when altering type of generated column"), + errdetail("Column \"%s\" is a generated column.", colName))); +errcode should be ERRCODE_FEATURE_NOT_SUPPORTED?
Although ERRCODE_INVALID_TABLE_DEFINITION is used for en error on changing
type of inherited column, I guess that is because it prevents from breaking
consistency between inherited and inheriting tables as a result of the command.
In this sense, maybe, ERRCODE_INVALID_COLUMN_DEFINITION is proper here, because
this check is to prevent inconsistency between columns in a tuple.
Yes, that was my thinking. I think of ERRCODE_FEATURE_NOT_SUPPORTED as
"we could add it in the future", but that does not seem to apply here.
also
CREATE TABLE gtest27 (
a int,
b text collate "C",
x text GENERATED ALWAYS AS ( b || '_2') STORED
);ALTER TABLE gtest27 ALTER COLUMN x TYPE int;
ERROR: column "x" cannot be cast automatically to type integer
HINT: You might need to specify "USING x::integer".should we do something for the errhint, since this specific errhint is wrong?
Yes. I think we don't have to output the hint message if we disallow USING
for generated columns. Or, it may be useful to allow only a simple cast
for the generated column instead of completely prohibiting USING.
Good catch. Here is an updated patch that fixes this.
This got me thinking whether disallowing USING here would actually
prevent some useful cases, like if you want to change the type of a
generated column and need to supply an explicit cast, as the hint
suggested. But this actually wouldn't work anyway, because later on it
will try to cast the generation expression, and that will fail in the
same way because it uses the same coerce_to_target_type() parameters.
So I think this patch won't break anything. Maybe what I'm describing
here could be implemented as a new feature, but I'm looking at this as a
bug fix right now.
Attachments:
v2-0001-Disallow-USING-clause-when-altering-type-of-gener.patchtext/plain; charset=UTF-8; name=v2-0001-Disallow-USING-clause-when-altering-type-of-gener.patchDownload
From 4ae50b2ee8602b39749bcc074f91ff0a0112848e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 22 Aug 2024 09:04:13 +0200
Subject: [PATCH v2] Disallow USING clause when altering type of generated
column
This does not make sense. It would write the output of the USING
clause into the converted column, which would violate the generation
expression. This adds a check to error out if this is specified.
There was a test for this, but that test errored out for a different
reason, so it was not effective.
Discussion: https://www.postgresql.org/message-id/flat/c7083982-69f4-4b14-8315-f9ddb20b9834%40eisentraut.org
---
src/backend/commands/tablecmds.c | 13 ++++++++++++-
src/test/regress/expected/generated.out | 3 ++-
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7a36db6af6d..e10e539152b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -12736,6 +12736,16 @@ ATPrepAlterColumnType(List **wqueue,
errmsg("cannot alter system column \"%s\"",
colName)));
+ /*
+ * Cannot specify USING when altering type of a generated column, because
+ * that would violate the generation expression.
+ */
+ if (attTup->attgenerated && def->cooked_default)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("cannot specify USING when altering type of generated column"),
+ errdetail("Column \"%s\" is a generated column.", colName)));
+
/*
* Don't alter inherited columns. At outer level, there had better not be
* any inherited definition; when recursing, we assume this was checked at
@@ -12813,10 +12823,11 @@ ATPrepAlterColumnType(List **wqueue,
errmsg("column \"%s\" cannot be cast automatically to type %s",
colName, format_type_be(targettype)),
/* translator: USING is SQL, don't translate it */
+ !attTup->attgenerated ?
errhint("You might need to specify \"USING %s::%s\".",
quote_identifier(colName),
format_type_with_typemod(targettype,
- targettypmod))));
+ targettypmod)) : 0));
}
/* Fix collations after all else */
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index 44058db7c1d..499072e14ca 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -1026,7 +1026,8 @@ SELECT * FROM gtest27;
(2 rows)
ALTER TABLE gtest27 ALTER COLUMN x TYPE boolean USING x <> 0; -- error
-ERROR: generation expression for column "x" cannot be cast automatically to type boolean
+ERROR: cannot specify USING when altering type of generated column
+DETAIL: Column "x" is a generated column.
ALTER TABLE gtest27 ALTER COLUMN x DROP DEFAULT; -- error
ERROR: column "x" of relation "gtest27" is a generated column
HINT: Use ALTER TABLE ... ALTER COLUMN ... DROP EXPRESSION instead.
--
2.46.0
On Thu, 22 Aug 2024 09:10:52 +0200
Peter Eisentraut <peter@eisentraut.org> wrote:
On 22.08.24 08:15, Yugo Nagata wrote:
On Thu, 22 Aug 2024 11:38:49 +0800
jian he <jian.universality@gmail.com> wrote:On Wed, Aug 21, 2024 at 4:57 PM Peter Eisentraut <peter@eisentraut.org> wrote:
+ /* + * Cannot specify USING when altering type of a generated column, because + * that would violate the generation expression. + */ + if (attTup->attgenerated && def->cooked_default) + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot specify USING when altering type of generated column"), + errdetail("Column \"%s\" is a generated column.", colName))); +errcode should be ERRCODE_FEATURE_NOT_SUPPORTED?
Although ERRCODE_INVALID_TABLE_DEFINITION is used for en error on changing
type of inherited column, I guess that is because it prevents from breaking
consistency between inherited and inheriting tables as a result of the command.
In this sense, maybe, ERRCODE_INVALID_COLUMN_DEFINITION is proper here, because
this check is to prevent inconsistency between columns in a tuple.Yes, that was my thinking. I think of ERRCODE_FEATURE_NOT_SUPPORTED as
"we could add it in the future", but that does not seem to apply here.
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("cannot specify USING when altering type of generated column"),
+ errdetail("Column \"%s\" is a generated column.", colName)));Do you thnik ERRCODE_INVALID_TABLE_DEFINITION is more proper than
ERRCODE_INVALID_COLUMN_DEFINITION in this case?
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
On 22.08.24 09:59, Yugo NAGATA wrote:
Although ERRCODE_INVALID_TABLE_DEFINITION is used for en error on changing
type of inherited column, I guess that is because it prevents from breaking
consistency between inherited and inheriting tables as a result of the command.
In this sense, maybe, ERRCODE_INVALID_COLUMN_DEFINITION is proper here, because
this check is to prevent inconsistency between columns in a tuple.Yes, that was my thinking. I think of ERRCODE_FEATURE_NOT_SUPPORTED as
"we could add it in the future", but that does not seem to apply here.+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot specify USING when altering type of generated column"), + errdetail("Column \"%s\" is a generated column.", colName)));Do you thnik ERRCODE_INVALID_TABLE_DEFINITION is more proper than
ERRCODE_INVALID_COLUMN_DEFINITION in this case?
COLUMN seems better here.
I copied TABLE from the "cannot alter system column" above, but maybe
that is a different situation.
On 22.08.24 10:49, Peter Eisentraut wrote:
On 22.08.24 09:59, Yugo NAGATA wrote:
Although ERRCODE_INVALID_TABLE_DEFINITION is used for en error on
changing
type of inherited column, I guess that is because it prevents from
breaking
consistency between inherited and inheriting tables as a result of
the command.
In this sense, maybe, ERRCODE_INVALID_COLUMN_DEFINITION is proper
here, because
this check is to prevent inconsistency between columns in a tuple.Yes, that was my thinking. I think of ERRCODE_FEATURE_NOT_SUPPORTED as
"we could add it in the future", but that does not seem to apply here.+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot specify USING when altering type of generated column"), + errdetail("Column \"%s\" is a generated column.", colName)));Do you thnik ERRCODE_INVALID_TABLE_DEFINITION is more proper than
ERRCODE_INVALID_COLUMN_DEFINITION in this case?COLUMN seems better here.
Committed and backpatched, with that adjustment.