From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: exclusion@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17792: MERGE uses uninitialized pointer and crashes when target tuple is updated concurrently
Date: Tue, 14 Feb 2023 20:19:37 +0100
Message-ID: <20230214191937.sri5zsqqogpidmxp@alvherre.pgsql> (raw)
In-Reply-To: <CAEZATCXHwmh7GEZ_z5g8otS4-J_VMaEobqkYNzPeyQu3VeYnHA@mail.gmail.com>
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.cindex 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;
/*
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: alvherre@alvh.no-ip.org, dean.a.rasheed@gmail.com, exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17792: MERGE uses uninitialized pointer and crashes when target tuple is updated concurrently
In-Reply-To: <20230214191937.sri5zsqqogpidmxp@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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