pg.ddx.io  pgsql-bugs@postgresql.org mailing list archive  

help / color / mirror / Atom feed

From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: exclusion@gmail.com
To: 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 12:29:33 +0100
Message-ID: <20230214112933.cshqgnzpcioedizy@alvherre.pgsql> (raw)
In-Reply-To: <17792-0f89452029662c36@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

view thread (9+ messages)  latest in thread

Message-ID: <20230214112933.cshqgnzpcioedizy@alvherre.pgsql>
Permalink:  ../20230214112933.cshqgnzpcioedizy@alvherre.pgsql/
Also on:    postgresql.org/message-id/20230214112933.cshqgnzpcioedizy@alvherre.pgsql

copy link · copy postgr.es · copy git am

reply

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, 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: <20230214112933.cshqgnzpcioedizy@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