pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17792: MERGE uses uninitialized pointer and crashes when target tuple is updated concurrently
9+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2023-03-13 9321c79c86 7d9a75713a Fix concurrent update issues with MERGE.
2023-02-15 fd923b5de9 5d8ec1b9f6 Don't rely on uninitialized value in MERGE / DELETE
* BUG #17792: MERGE uses uninitialized pointer and crashes when target tuple is updated concurrently
@ 2023-02-14 07:00 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 9+ messages in thread
From: PG Bug reporting form @ 2023-02-14 07:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
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).
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #17792: MERGE uses uninitialized pointer and crashes when target tuple is updated concurrently
@ 2023-02-14 11:29 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 2 replies; 9+ messages in thread
From: Alvaro Herrera @ 2023-02-14 11:29 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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/
Attachments:
merge-uninit.patch (text/x-diff, 579B) download
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 1ac65172e4..2a0e002dc9 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -3630,6 +3630,9 @@ ExecModifyTable(PlanState *pstate)
*/
ResetPerTupleExprContext(estate);
+ /* Decisions are made based on whether this is NULL for each tuple */
+ context.cpUpdateRetrySlot = NULL;
+
/*
* Reset per-tuple memory context used for processing on conflict and
* returning clauses, to free any expression evaluation storage
* Re: BUG #17792: MERGE uses uninitialized pointer and crashes when target tuple is updated concurrently
@ 2023-02-14 12:00 Alexander Lakhin <exclusion@gmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
1 sibling, 0 replies; 9+ messages in thread
From: Alexander Lakhin @ 2023-02-14 12:00 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #17792: MERGE uses uninitialized pointer and crashes when target tuple is updated concurrently
@ 2023-02-14 12:05 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
1 sibling, 1 reply; 9+ messages in thread
From: Dean Rasheed @ 2023-02-14 12:05 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #17792: MERGE uses uninitialized pointer and crashes when target tuple is updated concurrently
@ 2023-02-14 13:22 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Dean Rasheed @ 2023-02-14 13:22 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #17792: MERGE uses uninitialized pointer and crashes when target tuple is updated concurrently
@ 2023-02-14 19:19 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Alvaro Herrera @ 2023-02-14 19:19 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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)
Attachments:
merge-uninit-2.patch (text/x-diff, 2.0K) download
gpg: Firmado el mar 14 feb 2023 20:17:10 CET
gpg: usando RSA clave E2C96E4A9BCA7E92A8E3DA551C20ACB9D5C564AE
gpg: Firma correcta de "Álvaro Herrera <alvherre@alvh.no-ip.org>" [absoluta]
gpg: alias "Álvaro Herrera (PostgreSQL Global Development Group) <alvherre@postgresql.org>" [absoluta]
gpg: alias "Álvaro Herrera (2ndQuadrant) <alvherre@2ndQuadrant.com>" [absoluta]
commit 14242c4f5bebb21f945c3f8f43b02d34bb060807
Author: Alvaro Herrera <alvherre@alvh.no-ip.org> [Álvaro Herrera <alvherre@alvh.no-ip.org>]
AuthorDate: Tue Feb 14 19:26:51 2023 +0100
CommitDate: Tue Feb 14 20:17:10 2023 +0100
fix crash on mis-set cpUpdateRetrySlot
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 1ac65172e4..c4b845160d 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2967,21 +2967,20 @@ lmerge_matched:
*/
/*
- * If cpUpdateRetrySlot is set, ExecCrossPartitionUpdate()
- * must have detected that the tuple was concurrently
- * updated, so we restart the search for an appropriate
- * WHEN MATCHED clause to process the updated tuple.
+ * During an UPDATE, if cpUpdateRetrySlot is set, then
+ * ExecCrossPartitionUpdate() must have detected that the
+ * tuple was concurrently updated, so we restart the search
+ * for an appropriate WHEN MATCHED clause to process the
+ * updated tuple.
*
* In this case, ExecDelete() would already have performed
* EvalPlanQual() on the latest version of the tuple,
* which in turn would already have been loaded into
* ri_oldTupleSlot, so no need to do either of those
* things.
- *
- * XXX why do we not check the WHEN NOT MATCHED list in
- * this case?
*/
- if (!TupIsNull(context->cpUpdateRetrySlot))
+ if (commandType == CMD_UPDATE &&
+ !TupIsNull(context->cpUpdateRetrySlot))
goto lmerge_matched;
/*
* Re: BUG #17792: MERGE uses uninitialized pointer and crashes when target tuple is updated concurrently
@ 2023-02-15 09:41 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 9+ messages in thread
From: Dean Rasheed @ 2023-02-15 09:41 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #17792: MERGE uses uninitialized pointer and crashes when target tuple is updated concurrently
@ 2023-02-15 19:42 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Alvaro Herrera @ 2023-02-15 19:42 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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/
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #17792: MERGE uses uninitialized pointer and crashes when target tuple is updated concurrently
@ 2023-03-13 11:46 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 0 replies; 9+ messages in thread
From: Dean Rasheed @ 2023-03-13 11:46 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2023-03-13 11:46 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-02-14 07:00 BUG #17792: MERGE uses uninitialized pointer and crashes when target tuple is updated concurrently PG Bug reporting form <noreply@postgresql.org>
2023-02-14 11:29 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-14 12:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-02-14 12:05 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2023-02-14 13:22 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2023-02-14 19:19 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-15 09:41 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2023-02-15 19:42 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-03-13 11:46 ` Dean Rasheed <dean.a.rasheed@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox