Feedback-ID: ia2694551:Fastmail
Date: Tue, 14 Feb 2023 20:19:37 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17792: MERGE uses uninitialized pointer and crashes when
 target tuple is updated concurrently
Message-ID: <20230214191937.sri5zsqqogpidmxp@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="fd2oymvm7svqwr3o"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAEZATCXHwmh7GEZ_z5g8otS4-J_VMaEobqkYNzPeyQu3VeYnHA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20230214191937.sri5zsqqogpidmxp%40alvherre.pgsql>
Precedence: bulk


--fd2oymvm7svqwr3o
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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)

--fd2oymvm7svqwr3o
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment; filename="merge-uninit-2.patch"
Content-Transfer-Encoding: 8bit

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;
 
 					/*

--fd2oymvm7svqwr3o--