Thread: 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] [0]: https://www.postgresql.org/message-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]: > https://www.postgresql.org/message-id/CACJufxEGPYtFe79hbsMeOBOivfNnPRsw7Gjvk67m1x2MQggyiQ@mail.gmail.com > > -- Yugo Nagata <nagata@sraoss.co.jp>
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 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.