Thread: UPDATE on Domain Array that is based on a composite key crashes

On Tue, 19 Oct 2021 at 17:12, Zhihong Yu <zyu@yugabyte.com> wrote:
> On Tue, Oct 19, 2021 at 12:39 AM Onder Kalaci <onderk@microsoft.com> wrote:
>
>> Hi hackers,
>>
>>
>>
>> I couldn’t find a similar report to this one, so starting a new thread. I
>> can reproduce this on v14.0 as well as PostgreSQL 12.5 (not tried below
>> versions).
>>
>>
>>
>> Steps to reproduce:
>>
>>
>>
>> CREATE TYPE two_ints as (if1 int, if2 int);
>>
>> CREATE DOMAIN domain AS two_ints CHECK ((VALUE).if1 > 0);
>>
>> CREATE TABLE domain_indirection_test (f1 int, f3 domain, domain_array
>> domain[]);
>>
>> INSERT INTO domain_indirection_test (f1,f3.if1) VALUES (0, 1);
>>
>> UPDATE domain_indirection_test SET domain_array[0].if2 = 5;
>>
>> server closed the connection unexpectedly
>>
>>                 This probably means the server terminated abnormally
>>
>>                 before or while processing the request.
>>
>> The connection to the server was lost. Attempting reset: Failed.
>>
>> Time: 3.990 ms
>>
>
>  Hi,
>  It seems the following change would fix the crash:
>
> diff --git a/src/postgres/src/backend/executor/execExprInterp.c
> b/src/postgres/src/backend/executor/execExprInterp.c
> index 622cab9d4..50cb4f014 100644
> --- a/src/postgres/src/backend/executor/execExprInterp.c
> +++ b/src/postgres/src/backend/executor/execExprInterp.c
> @@ -3038,6 +3038,9 @@ ExecEvalFieldStoreDeForm(ExprState *state,
> ExprEvalStep *op, ExprContext *econte
>          HeapTupleHeader tuphdr;
>          HeapTupleData tmptup;
>
> +        if (DatumGetPointer(tupDatum) == NULL) {
> +            return;
> +        }
>          tuphdr = DatumGetHeapTupleHeader(tupDatum);
>          tmptup.t_len = HeapTupleHeaderGetDatumLength(tuphdr);
>          ItemPointerSetInvalid(&(tmptup.t_self));
>
> Here is the result after the update statement:
>
> # UPDATE domain_indirection_test SET domain_array[0].if2 = 5;
> UPDATE 1
> # select * from domain_indirection_test;
>  f1 |  f3  |  domain_array
> ----+------+----------------
>   0 | (1,) | [0:0]={"(,5)"}
> (1 row)
>

Yeah, it fixes the core dump.

However, When I test the patch, I find the update will replace all data
in `domain` if we only update one field.

postgres=# UPDATE domain_indirection_test SET domain_array[0].if2 = 5;
UPDATE 1
postgres=# select * from domain_indirection_test ;
 f1 |  f3  |  domain_array
----+------+----------------
  0 | (1,) | [0:0]={"(,5)"}
(1 row)

postgres=# UPDATE domain_indirection_test SET domain_array[0].if1 = 10;
UPDATE 1
postgres=# select * from domain_indirection_test ;
 f1 |  f3  |  domain_array
----+------+-----------------
  0 | (1,) | [0:0]={"(10,)"}
(1 row)


So I try to update all field in `domain`, and find only the last one will
be stored.

postgres=# UPDATE domain_indirection_test SET domain_array[0].if1 = 10, domain_array[0].if2 =  5;
UPDATE 1
postgres=# select * from domain_indirection_test ;
 f1 |  f3  |  domain_array
----+------+----------------
  0 | (1,) | [0:0]={"(,5)"}
(1 row)

postgres=# UPDATE domain_indirection_test SET domain_array[0].if2 = 10, domain_array[0].if1 =  5;
UPDATE 1
postgres=# select * from domain_indirection_test ;
 f1 |  f3  |  domain_array
----+------+----------------
  0 | (1,) | [0:0]={"(5,)"}
(1 row)


Does this worked as expected?  For me, For me, I think this is a bug.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Tue, 19 Oct 2021 at 23:17, Zhihong Yu <zyu@yugabyte.com> wrote:
> On Tue, Oct 19, 2021 at 2:12 AM Zhihong Yu <zyu@yugabyte.com> wrote:
>
>>
>>
>> On Tue, Oct 19, 2021 at 12:39 AM Onder Kalaci <onderk@microsoft.com>
>> wrote:
>>
>>> Hi hackers,
>>>
>>>
>>>
>>> I couldn’t find a similar report to this one, so starting a new thread. I
>>> can reproduce this on v14.0 as well as PostgreSQL 12.5 (not tried below
>>> versions).
>>>
>>>
>>>
>>> Steps to reproduce:
>>>
>>>
>>>
>>> CREATE TYPE two_ints as (if1 int, if2 int);
>>>
>>> CREATE DOMAIN domain AS two_ints CHECK ((VALUE).if1 > 0);
>>>
>>> CREATE TABLE domain_indirection_test (f1 int, f3 domain, domain_array
>>> domain[]);
>>>
>>> INSERT INTO domain_indirection_test (f1,f3.if1) VALUES (0, 1);
>>>
>>> UPDATE domain_indirection_test SET domain_array[0].if2 = 5;
>>>
>>> server closed the connection unexpectedly
>>>
>>>                 This probably means the server terminated abnormally
>>>
>>>                 before or while processing the request.
>>>
>>> The connection to the server was lost. Attempting reset: Failed.
>>>
>>> Time: 3.990 ms
>>>
>>> @:-!>
>>>
>>>
>>>
>>>
>>>
>>> The backtrace on PG 12.5 (As far as I remember, PG14 looks very similar) :
>>>
>>>
>>>
>>> (lldb) bt
>>>
>>> * thread #1, queue = 'com.apple.main-thread', stop reason =
>>> EXC_BAD_ACCESS (code=1, address=0x0)
>>>
>>>   * frame #0: 0x00000001036584b7
>>> postgres`pg_detoast_datum(datum=0x0000000000000000) at fmgr.c:1741:6
>>>
>>>     frame #1: 0x0000000103439a86
>>> postgres`ExecEvalFieldStoreDeForm(state=<unavailable>,
>>> op=0x00007f9212045df8, econtext=<unavailable>) at execExprInterp.c:3025:12
>>>
>>>     frame #2: 0x000000010343834e
>>> postgres`ExecInterpExpr(state=<unavailable>, econtext=<unavailable>,
>>> isnull=0x00007ffeec91fdc7) at execExprInterp.c:1337:4
>>>
>>>     frame #3: 0x000000010343742b
>>> postgres`ExecInterpExprStillValid(state=0x00007f921181db18,
>>> econtext=0x00007f921181d670, isNull=<unavailable>) at
>>> execExprInterp.c:1778:9
>>>
>>>     frame #4: 0x0000000103444e0d
>>> postgres`ExecEvalExprSwitchContext(state=0x00007f921181db18,
>>> econtext=0x00007f921181d670, isNull=<unavailable>) at executor.h:310:13
>>>
>>>     frame #5: 0x0000000103444cf0
>>> postgres`ExecProject(projInfo=0x00007f921181db10) at executor.h:344:9
>>>
>>>     frame #6: 0x0000000103444af6
>>> postgres`ExecScan(node=0x00007f921181d560, accessMtd=(postgres`SeqNext at
>>> nodeSeqscan.c:51), recheckMtd=(postgres`SeqRecheck at nodeSeqscan.c:90)) at
>>> execScan.c:239:12
>>>
>>>     frame #7: 0x0000000103461d17
>>> postgres`ExecSeqScan(pstate=<unavailable>) at nodeSeqscan.c:112:9
>>>
>>>     frame #8: 0x000000010344375c
>>> postgres`ExecProcNodeFirst(node=0x00007f921181d560) at execProcnode.c:445:9
>>>
>>>     frame #9: 0x000000010345eefe
>>> postgres`ExecProcNode(node=0x00007f921181d560) at executor.h:242:9
>>>
>>>     frame #10: 0x000000010345e74f
>>> postgres`ExecModifyTable(pstate=0x00007f921181d090) at
>>> nodeModifyTable.c:2079:14
>>>
>>>     frame #11: 0x000000010344375c
>>> postgres`ExecProcNodeFirst(node=0x00007f921181d090) at execProcnode.c:445:9
>>>
>>>     frame #12: 0x000000010343f25e
>>> postgres`ExecProcNode(node=0x00007f921181d090) at executor.h:242:9
>>>
>>>     frame #13: 0x000000010343d80d
>>> postgres`ExecutePlan(estate=0x00007f921181cd10,
>>> planstate=0x00007f921181d090, use_parallel_mode=<unavailable>,
>>> operation=CMD_UPDATE, sendTuples=false, numberTuples=0,
>>> direction=ForwardScanDirection, dest=0x00007f9211818c38,
>>> execute_once=<unavailable>) at execMain.c:1646:10
>>>
>>>     frame #14: 0x000000010343d745
>>> postgres`standard_ExecutorRun(queryDesc=0x00007f921180e310,
>>> direction=ForwardScanDirection, count=0, execute_once=true) at
>>> execMain.c:364:3
>>>
>>>     frame #15: 0x000000010343d67c
>>> postgres`ExecutorRun(queryDesc=0x00007f921180e310,
>>> direction=ForwardScanDirection, count=0, execute_once=<unavailable>) at
>>> execMain.c:308:3
>>>
>>>     frame #16: 0x00000001035784a8
>>> postgres`ProcessQuery(plan=<unavailable>, sourceText=<unavailable>,
>>> params=<unavailable>, queryEnv=0x0000000000000000, dest=<unavailable>,
>>> completionTag="") at pquery.c:161:2
>>>
>>>     frame #17: 0x0000000103577c5e
>>> postgres`PortalRunMulti(portal=0x00007f9215024110, isTopLevel=true,
>>> setHoldSnapshot=false, dest=0x00007f9211818c38, altdest=0x00007f9211818c38,
>>> completionTag="") at pquery.c:0
>>>
>>>     frame #18: 0x000000010357763d
>>> postgres`PortalRun(portal=0x00007f9215024110, count=9223372036854775807,
>>> isTopLevel=<unavailable>, run_once=<unavailable>, dest=0x00007f9211818c38,
>>> altdest=0x00007f9211818c38, completionTag="") at pquery.c:796:5
>>>
>>>     frame #19: 0x0000000103574f87
>>> postgres`exec_simple_query(query_string="UPDATE domain_indirection_test SET
>>> domain_array[0].if2 = 5;") at postgres.c:1215:10
>>>
>>>     frame #20: 0x00000001035746b8
>>> postgres`PostgresMain(argc=<unavailable>, argv=<unavailable>,
>>> dbname=<unavailable>, username=<unavailable>) at postgres.c:0
>>>
>>>     frame #21: 0x000000010350d712 postgres`BackendRun(port=<unavailable>)
>>> at postmaster.c:4494:2
>>>
>>>     frame #22: 0x000000010350cffa
>>> postgres`BackendStartup(port=<unavailable>) at postmaster.c:4177:3
>>>
>>>     frame #23: 0x000000010350c59c postgres`ServerLoop at
>>> postmaster.c:1725:7
>>>
>>>     frame #24: 0x000000010350ac8d postgres`PostmasterMain(argc=3,
>>> argv=0x00007f9210d049c0) at postmaster.c:1398:11
>>>
>>>     frame #25: 0x000000010347fbdd postgres`main(argc=<unavailable>,
>>> argv=<unavailable>) at main.c:228:3
>>>
>>>     frame #26: 0x00007fff204e8f3d libdyld.dylib`start + 1
>>>
>>>
>>>
>>>
>>>
>>> Thanks,
>>>
>>> Onder KALACI
>>>
>>> Software Engineer at Microsoft &
>>>
>>> Developing the Citus database extension for PostgreSQL
>>>
>>
>>  Hi,
>>  It seems the following change would fix the crash:
>>
>> diff --git a/src/postgres/src/backend/executor/execExprInterp.c
>> b/src/postgres/src/backend/executor/execExprInterp.c
>> index 622cab9d4..50cb4f014 100644
>> --- a/src/postgres/src/backend/executor/execExprInterp.c
>> +++ b/src/postgres/src/backend/executor/execExprInterp.c
>> @@ -3038,6 +3038,9 @@ ExecEvalFieldStoreDeForm(ExprState *state,
>> ExprEvalStep *op, ExprContext *econte
>>          HeapTupleHeader tuphdr;
>>          HeapTupleData tmptup;
>>
>> +        if (DatumGetPointer(tupDatum) == NULL) {
>> +            return;
>> +        }
>>          tuphdr = DatumGetHeapTupleHeader(tupDatum);
>>          tmptup.t_len = HeapTupleHeaderGetDatumLength(tuphdr);
>>          ItemPointerSetInvalid(&(tmptup.t_self));
>>
>> Here is the result after the update statement:
>>
>> # UPDATE domain_indirection_test SET domain_array[0].if2 = 5;
>> UPDATE 1
>> # select * from domain_indirection_test;
>>  f1 |  f3  |  domain_array
>> ----+------+----------------
>>   0 | (1,) | [0:0]={"(,5)"}
>> (1 row)
>>
>> Cheers
>>
> Hi,
> Here is the patch.

Thanks for your updated the patch.  A minor code style, we can remove the
braces when there is only one statement, this is more consenting with the
codebase.  Others looks good to me.

> If the new test should be placed in a different .sql file, please let me
> know.
>

> The update issue Japin mentioned seems to be orthogonal to the crash.
>

I start a new thread to discuss it [1].

[1] https://www.postgresql.org/message-id/MEYP282MB1669BED5CEFE711E00C7421EB6BD9@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM


--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

[ 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;