Re: UPDATE on Domain Array that is based on a composite key crashes - Mailing list pgsql-hackers

[ please do not quote the entire thread when replying ]

Zhihong Yu <zyu@yugabyte.com> writes:
> Here is the patch.

This patch seems quite misguided to me.  The proximate cause of
the crash is that we're arriving at ExecEvalFieldStoreDeForm with
*op->resnull and *op->resvalue both zero, which is a completely
invalid situation for a pass-by-reference datatype; so something
upstream of this messed up.  Even if there were an argument for
acting as though that were a valid NULL value, this patch fails to
do so; that'd require setting all the output fieldstore.nulls[]
entries to true, which you didn't.

Moreover, experiment quickly shows that the problem only shows up with
an array of domain over composite, not an array of plain composite.
The proposed patch doesn't seem to have anything to do with that
observation.

After some digging around, I see where the issue actually is:
the expression tree we're dealing with looks like

             {SUBSCRIPTINGREF 
             :refexpr 
                {VAR 
                }
             :refassgnexpr 
                {COERCETODOMAIN 
                :arg 
                   {FIELDSTORE 
                   :arg 
                      {CASETESTEXPR 
                      }
                   }
                }
             }

The array element we intend to replace has to be passed down to
the CaseTestExpr, but that isn't happening.  That's because
isAssignmentIndirectionExpr fails to recognize a tree like
this, so ExecInitSubscriptingRef doesn't realize it needs to
arrange for that.

I believe the attached is a correct fix.

            regards, tom lane

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 81b9d87bad..33ef39e2d4 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -3092,11 +3092,14 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *sbsref,
  * (We could use this in FieldStore too, but in that case passing the old
  * value is so cheap there's no need.)
  *
- * Note: it might seem that this needs to recurse, but it does not; the
- * CaseTestExpr, if any, will be directly the arg or refexpr of the top-level
- * node.  Nested-assignment situations give rise to expression trees in which
- * each level of assignment has its own CaseTestExpr, and the recursive
- * structure appears within the newvals or refassgnexpr field.
+ * Note: it might seem that this needs to recurse, but in most cases it does
+ * not; the CaseTestExpr, if any, will be directly the arg or refexpr of the
+ * top-level node.  Nested-assignment situations give rise to expression
+ * trees in which each level of assignment has its own CaseTestExpr, and the
+ * recursive structure appears within the newvals or refassgnexpr field.
+ * There is an exception, though: if the array is an array-of-domain, we will
+ * have a CoerceToDomain as the refassgnexpr, and we need to be able to look
+ * through that.
  */
 static bool
 isAssignmentIndirectionExpr(Expr *expr)
@@ -3117,6 +3120,12 @@ isAssignmentIndirectionExpr(Expr *expr)
         if (sbsRef->refexpr && IsA(sbsRef->refexpr, CaseTestExpr))
             return true;
     }
+    else if (IsA(expr, CoerceToDomain))
+    {
+        CoerceToDomain *cd = (CoerceToDomain *) expr;
+
+        return isAssignmentIndirectionExpr(cd->arg);
+    }
     return false;
 }

diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index 411d5c003e..a04bd00ac6 100644
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -512,6 +512,30 @@ LINE 1: update dposintatable set (f1[2])[1] = array[98];
 drop table dposintatable;
 drop domain posint cascade;
 NOTICE:  drop cascades to type dposinta
+-- Test arrays over domains of composite
+create type comptype as (cf1 int, cf2 int);
+create domain dcomptype as comptype check ((value).cf1 > 0);
+create table dcomptable (f1 dcomptype[]);
+insert into dcomptable values (null);
+update dcomptable set f1[1].cf2 = 5;
+table dcomptable;
+    f1
+----------
+ {"(,5)"}
+(1 row)
+
+update dcomptable set f1[1].cf1 = -1;  -- fail
+ERROR:  value for domain dcomptype violates check constraint "dcomptype_check"
+update dcomptable set f1[1].cf1 = 1;
+table dcomptable;
+    f1
+-----------
+ {"(1,5)"}
+(1 row)
+
+drop table dcomptable;
+drop type comptype cascade;
+NOTICE:  drop cascades to type dcomptype
 -- Test not-null restrictions
 create domain dnotnull varchar(15) NOT NULL;
 create domain dnull    varchar(15);
diff --git a/src/test/regress/sql/domain.sql b/src/test/regress/sql/domain.sql
index 549c0b5adf..bf48c53e9c 100644
--- a/src/test/regress/sql/domain.sql
+++ b/src/test/regress/sql/domain.sql
@@ -267,6 +267,23 @@ drop table dposintatable;
 drop domain posint cascade;


+-- Test arrays over domains of composite
+
+create type comptype as (cf1 int, cf2 int);
+create domain dcomptype as comptype check ((value).cf1 > 0);
+
+create table dcomptable (f1 dcomptype[]);
+insert into dcomptable values (null);
+update dcomptable set f1[1].cf2 = 5;
+table dcomptable;
+update dcomptable set f1[1].cf1 = -1;  -- fail
+update dcomptable set f1[1].cf1 = 1;
+table dcomptable;
+
+drop table dcomptable;
+drop type comptype cascade;
+
+
 -- Test not-null restrictions

 create domain dnotnull varchar(15) NOT NULL;