Check-out mutable functions in check constraints

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

Check-out mutable functions in check constraints

Kyotaro Horiguchi-4
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 <[hidden email]>
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

Reply | Threaded
Open this post in threaded view
|

Re: Check-out mutable functions in check constraints

Pavel Stehule
Hi

pá 12. 7. 2019 v 8:45 odesílatel Kyotaro Horiguchi <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Check-out mutable functions in check constraints

Tomas Vondra-4
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 <[hidden email]>
>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



Reply | Threaded
Open this post in threaded view
|

Re: Check-out mutable functions in check constraints

Tomas Vondra-4
In reply to this post by Kyotaro Horiguchi-4
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



Reply | Threaded
Open this post in threaded view
|

Re: Check-out mutable functions in check constraints

Pavel Stehule
In reply to this post by Tomas Vondra-4


pá 12. 7. 2019 v 13:11 odesílatel Tomas Vondra <[hidden email]> 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 <[hidden email]>
>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

Reply | Threaded
Open this post in threaded view
|

Re: Check-out mutable functions in check constraints

Tomas Vondra-4
On Fri, Jul 12, 2019 at 02:00:25PM +0200, Pavel Stehule wrote:

>pá 12. 7. 2019 v 13:11 odesílatel Tomas Vondra <[hidden email]>
>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 <
>> [hidden email]>
>> >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


Reply | Threaded
Open this post in threaded view
|

Re: Check-out mutable functions in check constraints

Tom Lane-2
Tomas Vondra <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Check-out mutable functions in check constraints

Tomas Vondra-4
On Fri, Jul 12, 2019 at 07:59:13PM -0400, Tom Lane wrote:

>Tomas Vondra <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Check-out mutable functions in check constraints

Tom Lane-2
Tomas Vondra <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Check-out mutable functions in check constraints

Robert Haas
On Sat, Jul 13, 2019 at 11:17 AM Tom Lane <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Check-out mutable functions in check constraints

Kyotaro Horiguchi-4
In reply to this post by Tomas Vondra-4
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 <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Check-out mutable functions in check constraints

Kyotaro Horiguchi-4
In reply to this post by Tom Lane-2
Hello, Thanks all!

At Sat, 13 Jul 2019 11:17:32 -0400, Tom Lane <[hidden email]> wrote in <[hidden email]>

> Tomas Vondra <[hidden email]> 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