Thread: Check-out mutable functions in check constraints
Hello. As mentioned in the following message: https://www.postgresql.org/message-id/20190712.150527.145133646.horikyota.ntt%40gmail.com Mutable function are allowed in check constraint expressions but it is not right. The attached is a proposed fix for it including regression test. Other "constraints vs xxxx" checks do not seem to be exercised but it would be another issue. regards. -- Kyotaro Horiguchi NTT Open Source Software Center From ed02bcf24a2fd666c80ab0b7ff63ccb370e26b03 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp> Date: Fri, 12 Jul 2019 15:35:20 +0900 Subject: [PATCH] Reject mutable functions in check constraints Check constraint expressions cannot contain mutable functions but it is not checked out. Fix it. --- src/backend/parser/parse_expr.c | 9 +++++++++ src/backend/parser/parse_func.c | 7 +++++++ src/test/regress/expected/create_table.out | 7 +++++++ src/test/regress/sql/create_table.sql | 3 +++ 4 files changed, 26 insertions(+) diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 97f535a2f0..2fd233bf18 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -2376,6 +2376,15 @@ transformMinMaxExpr(ParseState *pstate, MinMaxExpr *m) static Node * transformSQLValueFunction(ParseState *pstate, SQLValueFunction *svf) { + /* + * All SQL value functions are stable so we reject them in check + * constraint expressions. + */ + if (pstate->p_expr_kind == EXPR_KIND_CHECK_CONSTRAINT) + ereport(ERROR, + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), + errmsg("mutable functions are not allowed in check constraints"))); + /* * All we need to do is insert the correct result type and (where needed) * validate the typmod, so we just modify the node in-place. diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index 2a44b434a5..6ea2f0326d 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -271,6 +271,13 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs, &nvargs, &vatype, &declared_arg_types, &argdefaults); + /* mutable functions are not allowed in constraint expressions */ + if (pstate->p_expr_kind == EXPR_KIND_CHECK_CONSTRAINT && + func_volatile(funcid) != PROVOLATILE_IMMUTABLE) + ereport(ERROR, + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), + errmsg("mutable functions are not allowed in constraint expression"))); + cancel_parser_errposition_callback(&pcbstate); /* diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out index 262abf2bfb..aee192c2b5 100644 --- a/src/test/regress/expected/create_table.out +++ b/src/test/regress/expected/create_table.out @@ -331,6 +331,13 @@ CREATE TABLE default_expr_agg (a int DEFAULT (generate_series(1,3))); ERROR: set-returning functions are not allowed in DEFAULT expressions LINE 1: CREATE TABLE default_expr_agg (a int DEFAULT (generate_serie... ^ +-- invalid use of mutable function +CREATE TABLE mutable_incheckconstr_sqlvar (a text CHECK (a = CURRENT_USER)); +ERROR: mutable functions are not allowed in check constraints +CREATE TABLE mutable_incheckconstr_expr (a real CHECK (a = random())); +ERROR: mutable functions are not allowed in constraint expression +LINE 1: ...BLE mutable_incheckconstr_expr (a real CHECK (a = random()))... + ^ -- -- Partitioned tables -- diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql index 9c6d86a0bf..48e4e0c7db 100644 --- a/src/test/regress/sql/create_table.sql +++ b/src/test/regress/sql/create_table.sql @@ -317,6 +317,9 @@ CREATE TABLE default_expr_agg (a int DEFAULT (avg(1))); CREATE TABLE default_expr_agg (a int DEFAULT (select 1)); -- invalid use of set-returning function CREATE TABLE default_expr_agg (a int DEFAULT (generate_series(1,3))); +-- invalid use of mutable function +CREATE TABLE mutable_incheckconstr_sqlvar (a text CHECK (a = CURRENT_USER)); +CREATE TABLE mutable_incheckconstr_expr (a real CHECK (a = random())); -- -- Partitioned tables -- 2.16.3
Hi
pá 12. 7. 2019 v 8:45 odesílatel Kyotaro Horiguchi <horikyota.ntt@gmail.com> napsal:
Hello.
As mentioned in the following message:
https://www.postgresql.org/message-id/20190712.150527.145133646.horikyota.ntt%40gmail.com
Mutable function are allowed in check constraint expressions but
it is not right. The attached is a proposed fix for it including
regression test.
Other "constraints vs xxxx" checks do not seem to be exercised
but it would be another issue.
I think so this feature (although is correct) can breaks almost all applications - it is 20 year late.
Regards
Pavel
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Fri, Jul 12, 2019 at 08:55:20AM +0200, Pavel Stehule wrote: >Hi > >pá 12. 7. 2019 v 8:45 odesílatel Kyotaro Horiguchi <horikyota.ntt@gmail.com> >napsal: > >> Hello. >> >> As mentioned in the following message: >> >> >> https://www.postgresql.org/message-id/20190712.150527.145133646.horikyota.ntt%40gmail.com >> >> Mutable function are allowed in check constraint expressions but >> it is not right. The attached is a proposed fix for it including >> regression test. >> >> Other "constraints vs xxxx" checks do not seem to be exercised >> but it would be another issue. >> > >I think so this feature (although is correct) can breaks almost all >applications - it is 20 year late. > I'm not sure it actually breaks such appliations. Let's assume you have a mutable function (i.e. it may change return value even with the same parameters) and you use it in a CHECK constraint. Then I'm pretty sure your application is already broken in various ways and you just don't know it (sometimes it subtle, sometimes less so). If you have a function that actually is immutable and it's just not marked accordingly, then that only requires a single DDL to fix that during upgrade. I don't think that's a massive issue. That being said, I don't know whether fixing this is worth the hassle. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jul 12, 2019 at 03:44:58PM +0900, Kyotaro Horiguchi wrote: >Hello. > >As mentioned in the following message: > >https://www.postgresql.org/message-id/20190712.150527.145133646.horikyota.ntt%40gmail.com > >Mutable function are allowed in check constraint expressions but >it is not right. The attached is a proposed fix for it including >regression test. > I think the comment in parse_expr.c is wrong: /* * All SQL value functions are stable so we reject them in check * constraint expressions. */ if (pstate->p_expr_kind == EXPR_KIND_CHECK_CONSTRAINT) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("mutable functions are not allowed in check constraints"))); At first it claims SQL value functions are stable, but then rejects them with a message that they're mutable. Also, the other places use "cannot ..." messages: case EXPR_KIND_COLUMN_DEFAULT: err = _("cannot use column reference in DEFAULT expression"); break; so maybe these new checks should use the same style. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
pá 12. 7. 2019 v 13:11 odesílatel Tomas Vondra <tomas.vondra@2ndquadrant.com> napsal:
On Fri, Jul 12, 2019 at 08:55:20AM +0200, Pavel Stehule wrote:
>Hi
>
>pá 12. 7. 2019 v 8:45 odesílatel Kyotaro Horiguchi <horikyota.ntt@gmail.com>
>napsal:
>
>> Hello.
>>
>> As mentioned in the following message:
>>
>>
>> https://www.postgresql.org/message-id/20190712.150527.145133646.horikyota.ntt%40gmail.com
>>
>> Mutable function are allowed in check constraint expressions but
>> it is not right. The attached is a proposed fix for it including
>> regression test.
>>
>> Other "constraints vs xxxx" checks do not seem to be exercised
>> but it would be another issue.
>>
>
>I think so this feature (although is correct) can breaks almost all
>applications - it is 20 year late.
>
I'm not sure it actually breaks such appliations.
Let's assume you have a mutable function (i.e. it may change return value
even with the same parameters) and you use it in a CHECK constraint. Then
I'm pretty sure your application is already broken in various ways and you
just don't know it (sometimes it subtle, sometimes less so).
Years ago SQL functions was used for checks instead triggers - I am not sure if this pattern was in documentation or not, but surely there was not any warning against it.
You can see some documents with examples
CREATE OR REPLACE FUNCTION check_func(int)
RETURNS boolean AS $$
SELECT 1 FROM tab WHERE id = $1;
$$ LANGUAGE sql;
CREATE TABLE foo( ... id CHECK(check_func(id)));
If you have a function that actually is immutable and it's just not marked
accordingly, then that only requires a single DDL to fix that during
upgrade. I don't think that's a massive issue.
These functions are stable, and this patch try to prohibit it.
Regards
Pavel
That being said, I don't know whether fixing this is worth the hassle.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jul 12, 2019 at 02:00:25PM +0200, Pavel Stehule wrote: >pá 12. 7. 2019 v 13:11 odesílatel Tomas Vondra <tomas.vondra@2ndquadrant.com> >napsal: > >> On Fri, Jul 12, 2019 at 08:55:20AM +0200, Pavel Stehule wrote: >> >Hi >> > >> >pá 12. 7. 2019 v 8:45 odesílatel Kyotaro Horiguchi < >> horikyota.ntt@gmail.com> >> >napsal: >> > >> >> Hello. >> >> >> >> As mentioned in the following message: >> >> >> >> >> >> >> https://www.postgresql.org/message-id/20190712.150527.145133646.horikyota.ntt%40gmail.com >> >> >> >> Mutable function are allowed in check constraint expressions but >> >> it is not right. The attached is a proposed fix for it including >> >> regression test. >> >> >> >> Other "constraints vs xxxx" checks do not seem to be exercised >> >> but it would be another issue. >> >> >> > >> >I think so this feature (although is correct) can breaks almost all >> >applications - it is 20 year late. >> > >> >> I'm not sure it actually breaks such appliations. >> >> Let's assume you have a mutable function (i.e. it may change return value >> even with the same parameters) and you use it in a CHECK constraint. Then >> I'm pretty sure your application is already broken in various ways and you >> just don't know it (sometimes it subtle, sometimes less so). >> > >Years ago SQL functions was used for checks instead triggers - I am not >sure if this pattern was in documentation or not, but surely there was not >any warning against it. > >You can see some documents with examples > >CREATE OR REPLACE FUNCTION check_func(int) >RETURNS boolean AS $$ >SELECT 1 FROM tab WHERE id = $1; >$$ LANGUAGE sql; > >CREATE TABLE foo( ... id CHECK(check_func(id))); > Considering this does not work (e.g. because in READ COMMITTED mode you won't see the effects of uncommitted DELETE), I'd say this is a quite nice example of the breakage I mentioned before. You might add locking and make it somewhat safer, but there will still be plenty of holes (e.g. because you won't see new but not yet committed records). But it can cause issues e.g. with pg_dump [1]. So IMHO this is more an argument for adding the proposed check ... > > >> If you have a function that actually is immutable and it's just not marked >> accordingly, then that only requires a single DDL to fix that during >> upgrade. I don't think that's a massive issue. >> > >These functions are stable, and this patch try to prohibit it. > Yes, and the question is whether this is the right thing to do (I think it probably is). OTOH, even if we prohibit mutable functions in check constraints, people can still create triggers doing those checks (and shoot themselves in the foot that way). [1] https://www.postgresql.org/message-id/6753.1452274727%40sss.pgh.pa.us regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> On Fri, Jul 12, 2019 at 02:00:25PM +0200, Pavel Stehule wrote:
>>> Mutable function are allowed in check constraint expressions but
>>> it is not right. The attached is a proposed fix for it including
>>> regression test.
> Yes, and the question is whether this is the right thing to do (I think
> it probably is).
I'm pretty sure this change has been proposed before, and rejected before.
Has anybody excavated in the archives for prior discussions?
> OTOH, even if we prohibit mutable functions in check constraints, people
> can still create triggers doing those checks (and shoot themselves in
> the foot that way).
There are, and always will be, lots of ways to shoot yourself in the foot.
In the case at hand, I fear we might just encourage people to mark
functions as immutable when they really aren't --- which will make their
problems *worse* not better, because now other uses besides check
constraints will also be at risk of misoptimization.
regards, tom lane
On Fri, Jul 12, 2019 at 07:59:13PM -0400, Tom Lane wrote: >Tomas Vondra <tomas.vondra@2ndquadrant.com> writes: >> On Fri, Jul 12, 2019 at 02:00:25PM +0200, Pavel Stehule wrote: >>>> Mutable function are allowed in check constraint expressions but >>>> it is not right. The attached is a proposed fix for it including >>>> regression test. > >> Yes, and the question is whether this is the right thing to do (I think >> it probably is). > >I'm pretty sure this change has been proposed before, and rejected before. >Has anybody excavated in the archives for prior discussions? > Yes, I've done some quick searches like "volatile constraint" and so on. There are a couple of relevant discussions: 2004: https://www.postgresql.org/message-id/flat/0C3A1AEC-6BE4-11D8-9224-000A95C88220%40myrealbox.com 2010: https://www.postgresql.org/message-id/flat/12849.1277918175%40sss.pgh.pa.us#736c8ef9d7810c0bb85f495490fd40f5 But I don't think the conclusions are particularly clear. In the first thread you seem to agree with requiring immutable functions for check constraints (and triggers for one-time checks). The second thread ended up discussing some new related stuff in SQL standard. There may be other threads and I just haven't found them, of course. >There are, and always will be, lots of ways to shoot yourself in the foot. >In the case at hand, I fear we might just encourage people to mark >functions as immutable when they really aren't --- which will make their >problems *worse* not better, because now other uses besides check >constraints will also be at risk of misoptimization. > >> OTOH, even if we prohibit mutable functions in check constraints, people >> can still create triggers doing those checks (and shoot themselves in >> the foot that way). > >There are, and always will be, lots of ways to shoot yourself in the foot. >In the case at hand, I fear we might just encourage people to mark >functions as immutable when they really aren't --- which will make their >problems *worse* not better, because now other uses besides check >constraints will also be at risk of misoptimization. > True. -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes: > On Fri, Jul 12, 2019 at 07:59:13PM -0400, Tom Lane wrote: >> I'm pretty sure this change has been proposed before, and rejected before. >> Has anybody excavated in the archives for prior discussions? > Yes, I've done some quick searches like "volatile constraint" and so on. > There are a couple of relevant discussions: > 2004: https://www.postgresql.org/message-id/flat/0C3A1AEC-6BE4-11D8-9224-000A95C88220%40myrealbox.com > 2010: https://www.postgresql.org/message-id/flat/12849.1277918175%40sss.pgh.pa.us#736c8ef9d7810c0bb85f495490fd40f5 > But I don't think the conclusions are particularly clear. > In the first thread you seem to agree with requiring immutable functions > for check constraints (and triggers for one-time checks). The second > thread ended up discussing some new related stuff in SQL standard. Well, I think that second thread is very relevant here, because it correctly points out that we are *required by spec* to allow check constraints of the form CHECK(datecol <= CURRENT_DATE) and related tests. See the stuff about "retrospectively deterministic" predicates in SQL:2003 or later. I suppose you could imagine writing some messy logic that allowed the specific cases called out by the spec but not any other non-immutable function calls. But that just leaves us with an inconsistent restriction. If the spec is allowing this because it can be seen to be safe, why should we not allow other cases that the user has taken the trouble to prove to themselves are safe? (If their proof is wrong, well, it wouldn't be the first bug in anyone's SQL application.) regards, tom lane
On Sat, Jul 13, 2019 at 11:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > If the spec is allowing this because it can be seen > to be safe, why should we not allow other cases that the user has > taken the trouble to prove to themselves are safe? (If their proof is > wrong, well, it wouldn't be the first bug in anyone's SQL application.) Well said. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Mmm. # I eventually found messages sent to me stashed in unexpcted # place. I felt I was in a void space for these days.. That's # silly! Thank you for the comment. # Putting aside the appliability(?) of this check.. At Fri, 12 Jul 2019 13:14:57 +0200, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in <20190712111457.ekkcgx5mpkxl2ooh@development> > On Fri, Jul 12, 2019 at 03:44:58PM +0900, Kyotaro Horiguchi wrote: > >Hello. > > > >As mentioned in the following message: > > > >https://www.postgresql.org/message-id/20190712.150527.145133646.horikyota.ntt%40gmail.com > > > >Mutable function are allowed in check constraint expressions but > >it is not right. The attached is a proposed fix for it including > >regression test. > > > > I think the comment in parse_expr.c is wrong: > > /* > * All SQL value functions are stable so we reject them in check > * constraint expressions. > */ > if (pstate->p_expr_kind == EXPR_KIND_CHECK_CONSTRAINT) > ereport(ERROR, > (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), > errmsg("mutable functions are not allowed in check > constraints"))); > > At first it claims SQL value functions are stable, but then rejects > them > with a message that they're mutable. Isn't Stable mutable? By definition stable functions can return different values with the same input. But the message may be somewhat confusing for unaccostomed users. > Also, the other places use "cannot ..." messages: > > case EXPR_KIND_COLUMN_DEFAULT: > err = _("cannot use column reference in DEFAULT expression"); > break; > > so maybe these new checks should use the same style. It is following to messages like the follows: parse_func.c:2497 | case EXPR_KIND_CHECK_CONSTRAINT: | case EXPR_KIND_DOMAIN_CHECK: | err = _("set-returning functions are not allowed in check constraints"); Should we unify them? "are not allowed in" is used in parse_func.c and parse_agg.c, and "cannot use" is used in parse_expr.c for the same instruction. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Hello, Thanks all! At Sat, 13 Jul 2019 11:17:32 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <18372.1563031052@sss.pgh.pa.us> > Tomas Vondra <tomas.vondra@2ndquadrant.com> writes: > > On Fri, Jul 12, 2019 at 07:59:13PM -0400, Tom Lane wrote: > >> I'm pretty sure this change has been proposed before, and rejected before. > >> Has anybody excavated in the archives for prior discussions? > > > Yes, I've done some quick searches like "volatile constraint" and so on. > > There are a couple of relevant discussions: > > 2004: https://www.postgresql.org/message-id/flat/0C3A1AEC-6BE4-11D8-9224-000A95C88220%40myrealbox.com > > 2010: https://www.postgresql.org/message-id/flat/12849.1277918175%40sss.pgh.pa.us#736c8ef9d7810c0bb85f495490fd40f5 > > But I don't think the conclusions are particularly clear. > > In the first thread you seem to agree with requiring immutable functions > > for check constraints (and triggers for one-time checks). The second > > thread ended up discussing some new related stuff in SQL standard. > > Well, I think that second thread is very relevant here, because > it correctly points out that we are *required by spec* to allow > check constraints of the form CHECK(datecol <= CURRENT_DATE) and > related tests. See the stuff about "retrospectively deterministic" > predicates in SQL:2003 or later. > > I suppose you could imagine writing some messy logic that allowed the > specific cases called out by the spec but not any other non-immutable > function calls. But that just leaves us with an inconsistent > restriction. If the spec is allowing this because it can be seen > to be safe, why should we not allow other cases that the user has > taken the trouble to prove to themselves are safe? (If their proof is > wrong, well, it wouldn't be the first bug in anyone's SQL application.) > > regards, tom lane If, we have a CURRENT_DATE() that always returns UTC timestamp (or something like), then CURRENT_DATE()::text gives a local representation. We may have constraints using CURRENT_DATE() since it is truly immutable. I think the spec can be interpreted as that. regards. -- Kyotaro Horiguchi NTT Open Source Software Center