Thread
Commits
-
Disallow USING clause when altering type of generated column
- 4d68a043245a 18.0 landed
- 1c57ae795b3c 12.21 landed
- 7589d5c5b98d 13.17 landed
- ecd19a3cc187 14.14 landed
- cf49a606c416 15.9 landed
- 5867ee0056a2 16.5 landed
- fdbf7e46a46d 17.0 landed
-
Disallow USING clause when altering type of generated column
Peter Eisentraut <peter@eisentraut.org> — 2024-08-21T06:17:45Z
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] [0]: https://www.postgresql.org/message-id/CACJufxEGPYtFe79hbsMeOBOivfNnPRsw7Gjvk67m1x2MQggyiQ@mail.gmail.com
-
Re: Disallow USING clause when altering type of generated column
Yugo Nagata <nagata@sraoss.co.jp> — 2024-08-21T07:14:02Z
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]: > https://www.postgresql.org/message-id/CACJufxEGPYtFe79hbsMeOBOivfNnPRsw7Gjvk67m1x2MQggyiQ@mail.gmail.com > > -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: Disallow USING clause when altering type of generated column
Peter Eisentraut <peter@eisentraut.org> — 2024-08-21T08:57:48Z
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.
-
Re: Disallow USING clause when altering type of generated column
jian he <jian.universality@gmail.com> — 2024-08-22T03:38:49Z
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? -
Re: Disallow USING clause when altering type of generated column
Yugo Nagata <nagata@sraoss.co.jp> — 2024-08-22T06:15:31Z
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> -
Re: Disallow USING clause when altering type of generated column
Peter Eisentraut <peter@eisentraut.org> — 2024-08-22T07:10:52Z
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. -
Re: Disallow USING clause when altering type of generated column
Yugo Nagata <nagata@sraoss.co.jp> — 2024-08-22T07:59:47Z
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> -
Re: Disallow USING clause when altering type of generated column
Peter Eisentraut <peter@eisentraut.org> — 2024-08-22T08:49:22Z
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. -
Re: Disallow USING clause when altering type of generated column
Peter Eisentraut <peter@eisentraut.org> — 2024-08-29T07:15:51Z
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.