Обсуждение: BUG #17792: MERGE uses uninitialized pointer and crashes when target tuple is updated concurrently

The following bug has been logged on the website:

Bug reference:      17792
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 15.2
Operating system:   Ubuntu 22.04
Description:

When executing the following queries with valgrind:
echo "
CREATE TABLE source (sid integer);
INSERT INTO source VALUES (1);
CREATE TABLE target (tid integer, tval integer);
INSERT INTO target VALUES (1, 1);
" | psql

for ((i=1;i<=50;i++)); do
  echo "iteration $i"
  echo "
MERGE INTO target t USING source AS s ON t.tid = s.sid WHEN MATCHED THEN
UPDATE SET tval = 0;
MERGE INTO target t USING source AS s ON t.tid = s.sid WHEN MATCHED THEN
UPDATE SET tval = 0;
MERGE INTO target t USING source AS s ON t.tid = s.sid WHEN MATCHED THEN
UPDATE SET tval = 0;
  " | psql &
  echo "
MERGE INTO target t USING source AS s ON t.tid = s.sid WHEN MATCHED THEN
DELETE;
INSERT INTO target VALUES (1, 1);
  " | psql &
  wait
  grep '_suppression_' server.log && break
  grep 'runtime error: ' server.log && break
done

I get:
...
iteration 15
MERGE 1
MERGE 1
MERGE 1
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
connection to server was lost
   <insert_a_suppression_name_here>

server.log contains:
==00:00:00:28.417 799644== Conditional jump or move depends on uninitialised
value(s)
==00:00:00:28.417 799644==    at 0x43C67B: ExecMergeMatched
(nodeModifyTable.c:2992)
==00:00:00:28.417 799644==    by 0x43CCB4: ExecMerge
(nodeModifyTable.c:2750)
==00:00:00:28.417 799644==    by 0x43D72B: ExecModifyTable
(nodeModifyTable.c:3867)
==00:00:00:28.417 799644==    by 0x40E7F1: ExecProcNodeFirst
(execProcnode.c:464)
==00:00:00:28.417 799644==    by 0x406E19: ExecProcNode (executor.h:259)
==00:00:00:28.417 799644==    by 0x406E19: ExecutePlan (execMain.c:1636)
==00:00:00:28.417 799644==    by 0x406FF9: standard_ExecutorRun
(execMain.c:363)
==00:00:00:28.417 799644==    by 0x4070C5: ExecutorRun (execMain.c:307)
==00:00:00:28.417 799644==    by 0x5C5465: ProcessQuery (pquery.c:160)
==00:00:00:28.417 799644==    by 0x5C601A: PortalRunMulti (pquery.c:1277)
==00:00:00:28.417 799644==    by 0x5C65F3: PortalRun (pquery.c:791)
==00:00:00:28.417 799644==    by 0x5C27C3: exec_simple_query
(postgres.c:1250)
==00:00:00:28.417 799644==    by 0x5C465D: PostgresMain (postgres.c:4593)
==00:00:00:28.417 799644==  Uninitialised value was created by a stack
allocation
==00:00:00:28.417 799644==    at 0x43D189: ExecModifyTable
(nodeModifyTable.c:3572)
==00:00:00:28.417 799644==
{
   <insert_a_suppression_name_here>
...
==00:00:00:28.417 799644== Exit program on first error
(--exit-on-first-error=yes)
2023-02-14 08:43:28.826 MSK [799385] LOG:  server process (PID 799644)
exited with exit code 1
2023-02-14 08:43:28.826 MSK [799385] DETAIL:  Failed process was running:
MERGE INTO target t USING source AS s ON t.tid = s.sid WHEN MATCHED THEN
DELETE;

(With asan I get "nodeModifyTable.c:2992:11: runtime error: member access
within misaligned address 0x561fcdc53231 for type 'struct TupleTableSlot',
which requires 8 byte alignment", without valgrind/sanitizer just
SIGSEGV.)

At nodeModifyTable.c:2992 I see:
                    if (!TupIsNull(context->cpUpdateRetrySlot))

It looks like cpUpdateRetrySlot was not initialized on the context creation
in ExecModifyTable(), and ExecCrossPartitionUpdate(), where it could get a
value, just not called for this case.

Observed since the MERGE introduction (7103ebb7a).

On 2023-Feb-14, PG Bug reporting form wrote:

> When executing the following queries with valgrind:
> echo "
> CREATE TABLE source (sid integer);
> INSERT INTO source VALUES (1);
> CREATE TABLE target (tid integer, tval integer);
> INSERT INTO target VALUES (1, 1);
> " | psql

> At nodeModifyTable.c:2992 I see:
>                     if (!TupIsNull(context->cpUpdateRetrySlot))
> 
> It looks like cpUpdateRetrySlot was not initialized on the context creation
> in ExecModifyTable(), and ExecCrossPartitionUpdate(), where it could get a
> value, just not called for this case.

Hmm, yeah: because this is not a partitioned table, we are definitely
not expecting that cpUpdateRetrySlot will be set.  Can you still
reproduce the problem with the attached patch?


I'm not sure that the location of the initialization is best.  My first
impulse was to add it in line 3618, with the "Set global context" lines;
but then I think it's possible for one tuple of a partition to be routed
correctly and a later one that is concurrently updated suffer from an
improper value in cpUpdateRetrySlot.

Also, the comment is not great ...

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Hello Alvaro,
14.02.2023 14:29, Alvaro Herrera wrote:
> Hmm, yeah: because this is not a partitioned table, we are definitely
> not expecting that cpUpdateRetrySlot will be set.  Can you still
> reproduce the problem with the attached patch?
No, your patch fixes the issue.
Thanks for looking at this!

Best regards,
Alexander

On Tue, 14 Feb 2023 at 11:29, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> I'm not sure that the location of the initialization is best.  My first
> impulse was to add it in line 3618, with the "Set global context" lines;
> but then I think it's possible for one tuple of a partition to be routed
> correctly and a later one that is concurrently updated suffer from an
> improper value in cpUpdateRetrySlot.
>

Hmm, shouldn't it be initialised in ExecMergeMatched(), before line
2896, making the CMD_DELETE case match the CMD_UPDATE case? Otherwise
maybe an update action could be matched initially, try a
cross-partition update, setting cpUpdateRetrySlot due to a concurrent
update, and then upon retrying, a delete action might match.

Regards,
Dean

On Tue, 14 Feb 2023 at 12:05, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Tue, 14 Feb 2023 at 11:29, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > I'm not sure that the location of the initialization is best.  My first
> > impulse was to add it in line 3618, with the "Set global context" lines;
> > but then I think it's possible for one tuple of a partition to be routed
> > correctly and a later one that is concurrently updated suffer from an
> > improper value in cpUpdateRetrySlot.
> >
>
> Hmm, shouldn't it be initialised in ExecMergeMatched(), before line
> 2896, making the CMD_DELETE case match the CMD_UPDATE case? Otherwise
> maybe an update action could be matched initially, try a
> cross-partition update, setting cpUpdateRetrySlot due to a concurrent
> update, and then upon retrying, a delete action might match.
>

After trying to induce that, I realised that it doesn't appear to be
possible, because a delete after a failed update will always succeed,
because it has the target row locked by that point. So I think that it
will never need to retry more than once.

That said, it seems wrong to be checking cpUpdateRetrySlot after an
attempted delete anyway. Perhaps a better fix would be to just change
the check in ExecMergeMatched() to

    if (commandType == CMD_UPDATE && !TupIsNull(context->cpUpdateRetrySlot))
        goto lmerge_matched;

and update the preceding comment, since only an update should set
cpUpdateRetrySlot.

Regards,
Dean

On 2023-Feb-14, Dean Rasheed wrote:

> After trying to induce that, I realised that it doesn't appear to be
> possible, because a delete after a failed update will always succeed,
> because it has the target row locked by that point. So I think that it
> will never need to retry more than once.

Hmm, yeah, that makes sense.  Thanks for checking.

> That said, it seems wrong to be checking cpUpdateRetrySlot after an
> attempted delete anyway.

True.

> Perhaps a better fix would be to just change
> the check in ExecMergeMatched() to
> 
>     if (commandType == CMD_UPDATE && !TupIsNull(context->cpUpdateRetrySlot))
>         goto lmerge_matched;
> 
> and update the preceding comment, since only an update should set
> cpUpdateRetrySlot.

I agree, this looks to be a good fix.  However, I couldn't in a quick
try reproduce the problem, so I haven't been able to verify it.  I'll
try to do that early tomorrow.

(I also delete the XXX comment there.)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end." (2nd Commandment for C programmers)

On Tue, 14 Feb 2023 at 19:19, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> I agree, this looks to be a good fix.  However, I couldn't in a quick
> try reproduce the problem, so I haven't been able to verify it.  I'll
> try to do that early tomorrow.
>

I did some more testing, and the fix looks good.

> (I also delete the XXX comment there.)
>

That makes sense. It's a bit inconsistent (though not related to this
bug) that a cross-partition update will return OK if the tuple was
concurrently deleted, so merge will think that it updated the tuple
and not try an insert action, whereas for a normal update it will try
an insert action if the tuple was concurrently deleted. The thing that
seems wrong there is that ExecUpdateAct() sets updateCxt->updated =
true for a cross-partition update regardless of whether it actually
executed the insert half of the update/move. In theory, that flag
could be set to false so that merge would know if the tuple was
concurrently deleted, though it's not clear if it's worth it.

Regards,
Dean

On 2023-Feb-15, Dean Rasheed wrote:

> On Tue, 14 Feb 2023 at 19:19, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > I agree, this looks to be a good fix.  However, I couldn't in a quick
> > try reproduce the problem, so I haven't been able to verify it.  I'll
> > try to do that early tomorrow.
> 
> I did some more testing, and the fix looks good.

Thank you, I have pushed it.

> > (I also delete the XXX comment there.)
> 
> That makes sense. It's a bit inconsistent (though not related to this
> bug) that a cross-partition update will return OK if the tuple was
> concurrently deleted, so merge will think that it updated the tuple
> and not try an insert action, whereas for a normal update it will try
> an insert action if the tuple was concurrently deleted. The thing that
> seems wrong there is that ExecUpdateAct() sets updateCxt->updated =
> true for a cross-partition update regardless of whether it actually
> executed the insert half of the update/move. In theory, that flag
> could be set to false so that merge would know if the tuple was
> concurrently deleted, though it's not clear if it's worth it.

Hmm.  I wonder if this is just an inconsistency, or rather an outright
bug.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

On Wed, 15 Feb 2023 at 19:42, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2023-Feb-15, Dean Rasheed wrote:
>
> > That makes sense. It's a bit inconsistent (though not related to this
> > bug) that a cross-partition update will return OK if the tuple was
> > concurrently deleted, so merge will think that it updated the tuple
> > and not try an insert action, whereas for a normal update it will try
> > an insert action if the tuple was concurrently deleted. The thing that
> > seems wrong there is that ExecUpdateAct() sets updateCxt->updated =
> > true for a cross-partition update regardless of whether it actually
> > executed the insert half of the update/move. In theory, that flag
> > could be set to false so that merge would know if the tuple was
> > concurrently deleted, though it's not clear if it's worth it.
>
> Hmm.  I wonder if this is just an inconsistency, or rather an outright
> bug.
>

I just pushed a fix for bug #17809 that also fixes this. As part of
that, I greatly expanded the isolation tests, to try to cover a lot
more of these kinds of cases.

Regards,
Dean