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

From: Japin Li <japinli(at)hotmail(dot)com>
To: Zhihong Yu <zyu(at)yugabyte(dot)com>
Cc: Onder Kalaci <onderk(at)microsoft(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: UPDATE on Domain Array that is based on a composite key crashes
Date: 2021-10-19 10:33:36
Message-ID: MEYP282MB16697F0BE76CE6D0CFC3B079B6BD9@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


On Tue, 19 Oct 2021 at 17:12, Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
> On Tue, Oct 19, 2021 at 12:39 AM Onder Kalaci <onderk(at)microsoft(dot)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.

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2021-10-19 10:52:08 Re: Fixing build of MSVC with OpenSSL 3.0.0
Previous Message Ronan Dunklau 2021-10-19 09:51:40 Re: Delegating superuser tasks to new security roles (Was: Granting control of SUSET gucs to non-superusers)