Обсуждение: BUG #18449: Altering column type fails when an SQL routine depends on the column

The following bug has been logged on the website:

Bug reference:      18449
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 16.2
Operating system:   Ubuntu 22.04
Description:

The following script:
CREATE TABLE t(a int, b varchar);
CREATE FUNCTION f(i int) RETURNS text LANGUAGE SQL
RETURN (SELECT b FROM t WHERE a = $1);

ALTER TABLE t ALTER COLUMN b TYPE text;
fails with:
ERROR:  unexpected object depending on column: function f(integer)

on REL_14_STABLE .. master since e717a9a18.

PG Bug reporting form <noreply@postgresql.org> writes:
> The following script:
> CREATE TABLE t(a int, b varchar);
> CREATE FUNCTION f(i int) RETURNS text LANGUAGE SQL
> RETURN (SELECT b FROM t WHERE a = $1);

> ALTER TABLE t ALTER COLUMN b TYPE text;
> fails with:
> ERROR:  unexpected object depending on column: function f(integer)

> on REL_14_STABLE .. master since e717a9a18.

Thanks for the report.  It looks like most of the other hard cases
in RememberAllDependentForRebuilding just error out with code
along the lines of

                    ereport(ERROR,
                            (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                             errmsg("cannot alter type of a column used by a view or rule"),

I'm inclined to do likewise for functions.  We could imagine trying
to re-parse the function definition against the new column type,
but there are way too many ways for that to go wrong.  Just for
starters, there are possibly-security-grade hazards if the current
search_path isn't what it was when the function was created.  There's
no guarantee that we'd succeed anyway, eg the new column type might
not work for some function or operator that's applied to it, and if
not the resulting error message would likely be very surprising.

            regards, tom lane

Hello Tom,

28.04.2024 04:48, Tom Lane wrote:
> Thanks for the report.  It looks like most of the other hard cases
> in RememberAllDependentForRebuilding just error out with code
> along the lines of
>
>                      ereport(ERROR,
>                              (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>                               errmsg("cannot alter type of a column used by a view or rule"),
>
> I'm inclined to do likewise for functions.

I've discovered one more case, presumably as hard as the other ones:
CREATE TABLE t(a int);
CREATE PUBLICATION p FOR TABLE t WHERE (a > 0);

ALTER TABLE t ALTER COLUMN a TYPE bigint;
fails with:
ERROR:  unexpected object depending on column: publication of table t in publication p

Reproduced on REL_15_STABLE (52e4f0cd4) .. master.

> We could imagine trying
> to re-parse the function definition against the new column type,
> but there are way too many ways for that to go wrong.  Just for
> starters, there are possibly-security-grade hazards if the current
> search_path isn't what it was when the function was created.  There's
> no guarantee that we'd succeed anyway, eg the new column type might
> not work for some function or operator that's applied to it, and if
> not the resulting error message would likely be very surprising.

It looks like all these considerations apply to expressions defining row
filters for publications...

Best regards,
Alexander

Alexander Lakhin <exclusion@gmail.com> writes:
> I've discovered one more case, presumably as hard as the other ones:
> CREATE TABLE t(a int);
> CREATE PUBLICATION p FOR TABLE t WHERE (a > 0);
> ALTER TABLE t ALTER COLUMN a TYPE bigint;
> fails with:
> ERROR:  unexpected object depending on column: publication of table t in publication p

Hm.  We could fix this by introducing another single-purpose error
report, but I'm starting to think that that's failing to learn from
experience.  Who's to say that other column dependencies aren't
possible, now or in the future?  The only thing stopping us from
treating the default: case as a normal ERRCODE_FEATURE_NOT_SUPPORTED
error is that it might be hard to phrase the error message in a nice
way.  We already have a precedent for this being an acceptable
errdetail:

                    ereport(ERROR,
                            (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                             errmsg("cannot alter type of a column used in a policy definition"),
                             errdetail("%s depends on column \"%s\"",
                                       getObjectDescription(&foundObject, false),
                                       colName)));

It doesn't seem too awful to me to write the errmsg as

                             errmsg("cannot alter type of a column used in a %s",
                                    get_object_class_descr(foundObject.classid)),

This'd fall foul of English a/an grammar rules for some object class
names, so maybe we should phrase it a bit differently; but I'm sure
translated messages commit worse grammar violations all the time.

            regards, tom lane

01.05.2024 17:08, Tom Lane wrote:
> Hm.  We could fix this by introducing another single-purpose error
> report, but I'm starting to think that that's failing to learn from
> experience.  Who's to say that other column dependencies aren't
> possible, now or in the future?  The only thing stopping us from
> treating the default: case as a normal ERRCODE_FEATURE_NOT_SUPPORTED
> error is that it might be hard to phrase the error message in a nice
> way.  We already have a precedent for this being an acceptable
> errdetail:
>
>                      ereport(ERROR,
>                              (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>                               errmsg("cannot alter type of a column used in a policy definition"),
>                               errdetail("%s depends on column \"%s\"",
>                                         getObjectDescription(&foundObject, false),
>                                         colName)));
>
> It doesn't seem too awful to me to write the errmsg as
>
>                               errmsg("cannot alter type of a column used in a %s",
>                                      get_object_class_descr(foundObject.classid)),

I agree, though by doing that we'll loose the ability to detect obviously
wrong dependencies, say, when a role depends on a table column (if
INTERNAL_ERRORs expected to be handled as something abnormal),
but ATExecAlterColumnType() is hardly an appropriate place for such
detection anyway. So I would sacrifice this ability to make this function
simpler now and in the future.

Best regards,
Alexander

Alexander Lakhin <exclusion@gmail.com> writes:
> 01.05.2024 17:08, Tom Lane wrote:
>> It doesn't seem too awful to me to write the errmsg as
>> errmsg("cannot alter type of a column used in a %s",
>>        get_object_class_descr(foundObject.classId)),

> I agree, though by doing that we'll loose the ability to detect obviously
> wrong dependencies, say, when a role depends on a table column (if
> INTERNAL_ERRORs expected to be handled as something abnormal),
> but ATExecAlterColumnType() is hardly an appropriate place for such
> detection anyway. So I would sacrifice this ability to make this function
> simpler now and in the future.

I tried that, and it fell over on your example:

regression=# ALTER TABLE t ALTER COLUMN a TYPE bigint;
ERROR:  unrecognized class ID: 6106

6106 is pg_publication_rel, and apparently get_object_class_descr's
ambitions don't go far enough to include that.  Perhaps that should
be fixed, because if we have dependencies with that classId then
eventually somebody is going to wish to print debugging messages about
them.  However, that's probably not a project to undertake right now
with a release deadline looming.  For today, I'll just do something
exactly like 42b041243, except for publications.

            regards, tom lane

On 01.05.24 16:08, Tom Lane wrote:
> Hm.  We could fix this by introducing another single-purpose error
> report, but I'm starting to think that that's failing to learn from
> experience.  Who's to say that other column dependencies aren't
> possible, now or in the future?  The only thing stopping us from
> treating the default: case as a normal ERRCODE_FEATURE_NOT_SUPPORTED
> error is that it might be hard to phrase the error message in a nice
> way.  We already have a precedent for this being an acceptable
> errdetail:
> 
>                      ereport(ERROR,
>                              (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>                               errmsg("cannot alter type of a column used in a policy definition"),
>                               errdetail("%s depends on column \"%s\"",
>                                         getObjectDescription(&foundObject, false),
>                                         colName)));
> 
> It doesn't seem too awful to me to write the errmsg as
> 
>                               errmsg("cannot alter type of a column used in a %s",
>                                      get_object_class_descr(foundObject.classid)),
> 
> This'd fall foul of English a/an grammar rules for some object class
> names, so maybe we should phrase it a bit differently; but I'm sure
> translated messages commit worse grammar violations all the time.

Maybe something like

errmsg: cannot alter type of column %s because other objects depend on it

and then have the errdetails constructed similar to drop cascade.