Feedback-ID: ia2694551:Fastmail
Date: Tue, 14 Feb 2023 12:29:33 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: 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: <20230214112933.cshqgnzpcioedizy@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="ce5kdo4y4bapsfeo"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <17792-0f89452029662c36@postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/20230214112933.cshqgnzpcioedizy%40alvherre.pgsql>
Precedence: bulk


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

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/

--ce5kdo4y4bapsfeo
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="merge-uninit.patch"

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

--ce5kdo4y4bapsfeo--