Re: Error-safe user functions - Mailing list pgsql-hackers

Corey Huinker <corey.huinker@gmail.com> writes:
> On Sat, Dec 10, 2022 at 9:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> A fallback we can offer to anyone with such a problem is "write a
>> plpgsql function and wrap the potentially-failing bit in an exception
>> block".  Then they get to pay the cost of the subtransaction, while
>> we're not imposing one on everybody else.

> That exception block will prevent parallel plans. I'm not saying it isn't
> the best way forward for us, but wanted to make that side effect clear.

Hmm.  Apropos of that, I notice that domain_in is marked PARALLEL SAFE,
which seems like a bad idea if it could invoke not-so-parallel-safe
expressions.  Do we need to mark it less safe, and if so how much less?

Anyway, assuming that people are okay with the Not Our Problem approach,
the patch is pretty trivial, as attached.  I started to write an addition
to the CREATE DOMAIN man page recommending that domain CHECK constraints
not throw errors, but couldn't get past the bare recommendation.  Normally
I'd want to explain such a thing along the lines of "For example, X won't
work" ... but we don't yet have any committed features that depend on
this.  I'm inclined to leave it like that for now.  If we don't remember
to fix it once we do have some features, I'm sure somebody will ask a
question about it eventually.

            regards, tom lane

diff --git a/doc/src/sgml/ref/create_domain.sgml b/doc/src/sgml/ref/create_domain.sgml
index 82a0b87492..73f9f28d6c 100644
--- a/doc/src/sgml/ref/create_domain.sgml
+++ b/doc/src/sgml/ref/create_domain.sgml
@@ -239,6 +239,11 @@ INSERT INTO tab (domcol) VALUES ((SELECT domcol FROM tab WHERE false));
    DOMAIN</command>), adjust the function definition, and re-add the
    constraint, thereby rechecking it against stored data.
   </para>
+
+  <para>
+   It's also good practice to ensure that domain <literal>CHECK</literal>
+   expressions will not throw errors.
+  </para>
  </refsect1>

  <refsect1>
diff --git a/src/backend/utils/adt/domains.c b/src/backend/utils/adt/domains.c
index 3de0cb01a2..99aeaddb5d 100644
--- a/src/backend/utils/adt/domains.c
+++ b/src/backend/utils/adt/domains.c
@@ -126,9 +126,14 @@ domain_state_setup(Oid domainType, bool binary, MemoryContext mcxt)
  * This is roughly similar to the handling of CoerceToDomain nodes in
  * execExpr*.c, but we execute each constraint separately, rather than
  * compiling them in-line within a larger expression.
+ *
+ * If escontext points to an ErrorStateContext, any failures are reported
+ * there, otherwise they are ereport'ed.  Note that we do not attempt to do
+ * soft reporting of errors raised during execution of CHECK constraints.
  */
 static void
-domain_check_input(Datum value, bool isnull, DomainIOData *my_extra)
+domain_check_input(Datum value, bool isnull, DomainIOData *my_extra,
+                   Node *escontext)
 {
     ExprContext *econtext = my_extra->econtext;
     ListCell   *l;
@@ -144,11 +149,14 @@ domain_check_input(Datum value, bool isnull, DomainIOData *my_extra)
         {
             case DOM_CONSTRAINT_NOTNULL:
                 if (isnull)
-                    ereport(ERROR,
+                {
+                    errsave(escontext,
                             (errcode(ERRCODE_NOT_NULL_VIOLATION),
                              errmsg("domain %s does not allow null values",
                                     format_type_be(my_extra->domain_type)),
                              errdatatype(my_extra->domain_type)));
+                    goto fail;
+                }
                 break;
             case DOM_CONSTRAINT_CHECK:
                 {
@@ -179,13 +187,16 @@ domain_check_input(Datum value, bool isnull, DomainIOData *my_extra)
                     econtext->domainValue_isNull = isnull;

                     if (!ExecCheck(con->check_exprstate, econtext))
-                        ereport(ERROR,
+                    {
+                        errsave(escontext,
                                 (errcode(ERRCODE_CHECK_VIOLATION),
                                  errmsg("value for domain %s violates check constraint \"%s\"",
                                         format_type_be(my_extra->domain_type),
                                         con->name),
                                  errdomainconstraint(my_extra->domain_type,
                                                      con->name)));
+                        goto fail;
+                    }
                     break;
                 }
             default:
@@ -200,6 +211,7 @@ domain_check_input(Datum value, bool isnull, DomainIOData *my_extra)
      * per-tuple memory.  This avoids leaking non-memory resources, if
      * anything in the expression(s) has any.
      */
+fail:
     if (econtext)
         ReScanExprContext(econtext);
 }
@@ -213,6 +225,7 @@ domain_in(PG_FUNCTION_ARGS)
 {
     char       *string;
     Oid            domainType;
+    Node       *escontext = fcinfo->context;
     DomainIOData *my_extra;
     Datum        value;

@@ -245,15 +258,18 @@ domain_in(PG_FUNCTION_ARGS)
     /*
      * Invoke the base type's typinput procedure to convert the data.
      */
-    value = InputFunctionCall(&my_extra->proc,
-                              string,
-                              my_extra->typioparam,
-                              my_extra->typtypmod);
+    if (!InputFunctionCallSafe(&my_extra->proc,
+                               string,
+                               my_extra->typioparam,
+                               my_extra->typtypmod,
+                               escontext,
+                               &value))
+        PG_RETURN_NULL();

     /*
      * Do the necessary checks to ensure it's a valid domain value.
      */
-    domain_check_input(value, (string == NULL), my_extra);
+    domain_check_input(value, (string == NULL), my_extra, escontext);

     if (string == NULL)
         PG_RETURN_NULL();
@@ -309,7 +325,7 @@ domain_recv(PG_FUNCTION_ARGS)
     /*
      * Do the necessary checks to ensure it's a valid domain value.
      */
-    domain_check_input(value, (buf == NULL), my_extra);
+    domain_check_input(value, (buf == NULL), my_extra, NULL);

     if (buf == NULL)
         PG_RETURN_NULL();
@@ -349,7 +365,7 @@ domain_check(Datum value, bool isnull, Oid domainType,
     /*
      * Do the necessary checks to ensure it's a valid domain value.
      */
-    domain_check_input(value, isnull, my_extra);
+    domain_check_input(value, isnull, my_extra, NULL);
 }

 /*
diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index 73b010f6ed..25f6bb9e1f 100644
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -87,6 +87,56 @@ drop domain domainvarchar restrict;
 drop domain domainnumeric restrict;
 drop domain domainint4 restrict;
 drop domain domaintext;
+-- Test non-error-throwing input
+create domain positiveint int4 check(value > 0);
+create domain weirdfloat float8 check((1 / value) < 10);
+select pg_input_is_valid('1', 'positiveint');
+ pg_input_is_valid
+-------------------
+ t
+(1 row)
+
+select pg_input_is_valid('junk', 'positiveint');
+ pg_input_is_valid
+-------------------
+ f
+(1 row)
+
+select pg_input_is_valid('-1', 'positiveint');
+ pg_input_is_valid
+-------------------
+ f
+(1 row)
+
+select pg_input_error_message('junk', 'positiveint');
+            pg_input_error_message
+-----------------------------------------------
+ invalid input syntax for type integer: "junk"
+(1 row)
+
+select pg_input_error_message('-1', 'positiveint');
+                           pg_input_error_message
+----------------------------------------------------------------------------
+ value for domain positiveint violates check constraint "positiveint_check"
+(1 row)
+
+select pg_input_error_message('junk', 'weirdfloat');
+                 pg_input_error_message
+--------------------------------------------------------
+ invalid input syntax for type double precision: "junk"
+(1 row)
+
+select pg_input_error_message('0.01', 'weirdfloat');
+                          pg_input_error_message
+--------------------------------------------------------------------------
+ value for domain weirdfloat violates check constraint "weirdfloat_check"
+(1 row)
+
+-- We currently can't trap errors raised in the CHECK expression itself
+select pg_input_error_message('0', 'weirdfloat');
+ERROR:  division by zero
+drop domain positiveint;
+drop domain weirdfloat;
 -- Test domains over array types
 create domain domainint4arr int4[1];
 create domain domainchar4arr varchar(4)[2][3];
diff --git a/src/test/regress/sql/domain.sql b/src/test/regress/sql/domain.sql
index f2ca1fb675..1558bd9a33 100644
--- a/src/test/regress/sql/domain.sql
+++ b/src/test/regress/sql/domain.sql
@@ -69,6 +69,25 @@ drop domain domainint4 restrict;
 drop domain domaintext;


+-- Test non-error-throwing input
+
+create domain positiveint int4 check(value > 0);
+create domain weirdfloat float8 check((1 / value) < 10);
+
+select pg_input_is_valid('1', 'positiveint');
+select pg_input_is_valid('junk', 'positiveint');
+select pg_input_is_valid('-1', 'positiveint');
+select pg_input_error_message('junk', 'positiveint');
+select pg_input_error_message('-1', 'positiveint');
+select pg_input_error_message('junk', 'weirdfloat');
+select pg_input_error_message('0.01', 'weirdfloat');
+-- We currently can't trap errors raised in the CHECK expression itself
+select pg_input_error_message('0', 'weirdfloat');
+
+drop domain positiveint;
+drop domain weirdfloat;
+
+
 -- Test domains over array types

 create domain domainint4arr int4[1];