Thread: 回复：[Internet]Re: [PATCH] Prevent replacement of a function if it's used in an index expression and is not IMMUTABLE
回复：[Internet]Re: [PATCH] Prevent replacement of a function if it's used in an index expression and is not IMMUTABLE
From
"sundayjiang(蒋浩天)"
Date:
Hi hackers,
Thanks for the valuable feedback on my previous patch.
Based on the review comments, I have updated the patch as follows:
- Limited the error trigger only to the case where the original function is IMMUTABLE but the new definition is not.
- Changed the error code to ERRCODE_FEATURE_NOT_SUPPORTED for better semantics.
- Improved code indentation and style to align with project conventions.
- Added a regression test case in create_function_sql.sql to verify the new behavior.
The full updated patch is attached below.
Please take a look at this revised version. I look forward to your feedback and suggestions.
Best regards,
xiaojiluo (Tencent Yunding Lab)
Thanks for the valuable feedback on my previous patch.
Based on the review comments, I have updated the patch as follows:
- Limited the error trigger only to the case where the original function is IMMUTABLE but the new definition is not.
- Changed the error code to ERRCODE_FEATURE_NOT_SUPPORTED for better semantics.
- Improved code indentation and style to align with project conventions.
- Added a regression test case in create_function_sql.sql to verify the new behavior.
The full updated patch is attached below.
Please take a look at this revised version. I look forward to your feedback and suggestions.
Best regards,
xiaojiluo (Tencent Yunding Lab)
jian he<jian.universality@gmail.com> 在 2025年7月9日 周三 10:34 写道：
On Mon, Jun 30, 2025 at 5:34 PM sundayjiang(蒋浩天)
<sundayjiang@tencent.com> wrote:
>
> The purpose of this patch is to prevent replacing a function via `CREATE OR REPLACE FUNCTION` with a new definition that is not marked as `IMMUTABLE`, if the existing function is referenced by an index expression.
>
> Replacing such functions may lead to index corruption or runtime semantic inconsistencies, especially when the function’s output is not stable for the same input.
>
> If a function is used in an index, it can only be replaced if it is declared as `IMMUTABLE`.
>
looking at the right above code ``if (oldproc->prokind != prokind)``
+ if(oldproc->prokind == PROKIND_FUNCTION && volatility !=
PROVOLATILE_IMMUTABLE){
we can change it to
+ if(prokind == PROKIND_FUNCTION && oldproc->provolatile ==
PROVOLATILE_IMMUTABLE &&
+ volatility != PROVOLATILE_IMMUTABLE)
+ {
curly brace generally begins with a new line.
if (index_found)
+ ereport(ERROR,
+ (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
+ errmsg("cannot replace function \"%s\" with a non-IMMUTABLE function
because it is used by an index",
+ procedureName)));
Here, errcode ERRCODE_FEATURE_NOT_SUPPORTED would be more appropriate.
you can add a simple test in src/test/regress/sql/create_function_sql.sql
for example:
CREATE OR REPLACE FUNCTION get_a_int(int default 2) RETURNS int
IMMUTABLE AS 'select $1' LANGUAGE sql;
create table t1(a int);
create index on t1((get_a_int(a)));
CREATE OR REPLACE FUNCTION get_a_int(int default 2) RETURNS int AS
'select $1' LANGUAGE sql;
<sundayjiang@tencent.com> wrote:
>
> The purpose of this patch is to prevent replacing a function via `CREATE OR REPLACE FUNCTION` with a new definition that is not marked as `IMMUTABLE`, if the existing function is referenced by an index expression.
>
> Replacing such functions may lead to index corruption or runtime semantic inconsistencies, especially when the function’s output is not stable for the same input.
>
> If a function is used in an index, it can only be replaced if it is declared as `IMMUTABLE`.
>
looking at the right above code ``if (oldproc->prokind != prokind)``
+ if(oldproc->prokind == PROKIND_FUNCTION && volatility !=
PROVOLATILE_IMMUTABLE){
we can change it to
+ if(prokind == PROKIND_FUNCTION && oldproc->provolatile ==
PROVOLATILE_IMMUTABLE &&
+ volatility != PROVOLATILE_IMMUTABLE)
+ {
curly brace generally begins with a new line.
if (index_found)
+ ereport(ERROR,
+ (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
+ errmsg("cannot replace function \"%s\" with a non-IMMUTABLE function
because it is used by an index",
+ procedureName)));
Here, errcode ERRCODE_FEATURE_NOT_SUPPORTED would be more appropriate.
you can add a simple test in src/test/regress/sql/create_function_sql.sql
for example:
CREATE OR REPLACE FUNCTION get_a_int(int default 2) RETURNS int
IMMUTABLE AS 'select $1' LANGUAGE sql;
create table t1(a int);
create index on t1((get_a_int(a)));
CREATE OR REPLACE FUNCTION get_a_int(int default 2) RETURNS int AS
'select $1' LANGUAGE sql;
Attachment
Re: 回复：[Internet]Re: [PATCH] Prevent replacement of a function if it's used in an index expression and is not IMMUTABLE
From
Tom Lane
Date:
"=?utf-8?B?c3VuZGF5amlhbmco6JKL5rWp5aSpKQ==?=" <sundayjiang@tencent.com> writes:
> > The purpose of this patch is to prevent replacing a function via `CREATE OR REPLACE FUNCTION` with a new definition
thatis not marked as `IMMUTABLE`, if the existing function is referenced by an index expression.
> >
> > Replacing such functions may lead to index corruption or runtime semantic inconsistencies, especially when the
function’soutput is not stable for the same input.
TBH, I find this proposal to be useless nannyism. Replacing a
function that is used in an index is problematic if you change its
behavior (that is, its actual output for given inputs) in any way.
Whether it's marked IMMUTABLE is a very minor side point.
If we could detect whether the new definition results in different
behavior, that would be peachy ... but it's equivalent to solving
Turing's halting problem, so I doubt we're going to get there.
So the only really practical thing we could do about that would be
to forbid CREATE OR REPLACE altogether on functions used in indexes.
That proposal is never going to fly, however.
Another way in which the proposed patch is just a band-aid is that
it doesn't detect indirect dependencies, eg if function f() used
in some index calls function g() and you redefine function g().
So as far as I can see, this patch is slowing down function
redefinition in exchange for not much. We basically have to
trust that people who use user-defined functions in indexes
are aware of the hazard and don't change such functions in
ways that will break their indexes, or at least remember to
REINDEX afterwards. The portion of that hazard that we can
detect automatically is so small that I think we're just
fooling ourselves (and others) to try at all.
regards, tom lane
Re: 回复：[Internet]Re: [PATCH] Prevent replacement of a function if it's used in an index expression and is not IMMUTABLE
From
"David G. Johnston"
Date:
On Wed, Jul 9, 2025 at 9:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
"sundayjiang(蒋浩天)" <sundayjiang@tencent.com> writes:
> > The purpose of this patch is to prevent replacing a function via `CREATE OR REPLACE FUNCTION` with a new definition that is not marked as `IMMUTABLE`, if the existing function is referenced by an index expression.
> >
> > Replacing such functions may lead to index corruption or runtime semantic inconsistencies, especially when the function’s output is not stable for the same input.
TBH, I find this proposal to be useless nannyism. Replacing a
function that is used in an index is problematic if you change its
behavior (that is, its actual output for given inputs) in any way.
Whether it's marked IMMUTABLE is a very minor side point.
Isn't preventing a dump-restore hazard sufficient reason to do this? The now-volatile function will be dumped as such and the create index during the restore will fail because of that.
David J.
Re: 回复：[Internet]Re: [PATCH] Prevent replacement of a function if it's used in an index expression and is not IMMUTABLE
From
Tom Lane
Date:
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Wed, Jul 9, 2025 at 9:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> TBH, I find this proposal to be useless nannyism.
> Isn't preventing a dump-restore hazard sufficient reason to do this?
No, I don't think so. If you're not being very careful about revising
functions used in indexes, you are going to have problems a lot sooner
than some future dump/restore cycle.
regards, tom lane
Re: 回复：[Internet]Re: [PATCH] Prevent replacement of a function if it's used in an index expression and is not IMMUTABLE
From
"David G. Johnston"
Date:
On Wed, Jul 9, 2025 at 12:11 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Wed, Jul 9, 2025 at 9:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> TBH, I find this proposal to be useless nannyism.
> Isn't preventing a dump-restore hazard sufficient reason to do this?
No, I don't think so. If you're not being very careful about revising
functions used in indexes, you are going to have problems a lot sooner
than some future dump/restore cycle.
Then probably this patch can just update the create index documentation to say "create or replace it" instead of just "create it". The fact that during 'update' (replace) existing non-implied settings can be replaced with implied ones is a beginner/inattentive foot-gun. We do make the point clearly in create function but it seems worthwhile to reinforce it here too.
"To use a user-defined function in an index expression or WHERE clause, remember to mark the function immutable when you create [or replace] it."
David J.