Feedback-ID: ia2694551:Fastmail
Date: Tue, 12 Dec 2023 18:11:44 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, feichanghong@qq.com,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18241: PushTransaction may cause Standby to execute
 ItemIdMarkDead
Message-ID: <202312121711.tzjyb5yeb3fa@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZXggXVwX6F6gWZDq@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/202312121711.tzjyb5yeb3fa%40alvherre.pgsql>
Precedence: bulk

On 2023-Dec-12, Michael Paquier wrote:

> An issue if that this could cause problems if you do catalog
> scans, which may explain a few bugs I recall seeing over the years,
> even if these did not directly mention the use of ssavepoints.  I'd
> need to double-check the archives.  If going through a driver layer
> like the ODBC driver that enforces savepoints for each query, that
> would be bad. 

What a horrible bug.  Thanks for pushing the fix.  It looks OK to me:
nowhere else do we create a TransactionStateData that would need the
initialization.

I wonder if this can cause data corruption, if you happen to have a
broken standby that improperly kills some index entry and is later
promoted.  Maybe this bug explains these mysterious cases where an index
seems to lack a pointer to some heap tuple for no known reason --
especially notorious when you try to REINDEX a unique index and that
turns out not to work due to duplicates.

I would be happier if the order of initialization of the struct member
followed the order to the struct, with comments to note the missing
members.  That might make it more visible to future patches that would
add more members.  Something like this:

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 8442c5e6a7..b5874df821 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5291,17 +5291,25 @@ PushTransaction(void)
 	 */
 	s->fullTransactionId = InvalidFullTransactionId;	/* until assigned */
 	s->subTransactionId = currentSubTransactionId;
-	s->parent = p;
-	s->nestingLevel = p->nestingLevel + 1;
-	s->gucNestLevel = NewGUCNestLevel();
+	/* s->name = NULL; */
 	s->savepointLevel = p->savepointLevel;
 	s->state = TRANS_DEFAULT;
 	s->blockState = TBLOCK_SUBBEGIN;
+	s->nestingLevel = p->nestingLevel + 1;
+	s->gucNestLevel = NewGUCNestLevel();
+	/* s->curTransactionContext = NULL; */
+	/* s->curTransactionOwner = NULL */
+	/* s->childXids = NULL; */
+	/* s->nChildXids = 0; */
+	/* s->maxChildXids = 0; */
 	GetUserIdAndSecContext(&s->prevUser, &s->prevSecContext);
 	s->prevXactReadOnly = XactReadOnly;
 	s->startedInRecovery = p->startedInRecovery;
+	/* s->didLogXid = false; */
 	s->parallelModeLevel = 0;
+	/* s->chain = false; */
 	s->topXidLogged = false;
+	s->parent = p;
 
 	CurrentTransactionState = s;
 

Also, the way this function checks for overflow is strange.  Why
increment then check, leading to a forced free and decrement, instead of
just testing and throwing error right away?  It's less code:

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 8442c5e6a7..fac9141b16 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5272,18 +5272,14 @@ PushTransaction(void)
 		MemoryContextAllocZero(TopTransactionContext,
 							   sizeof(TransactionStateData));
 
-	/*
-	 * Assign a subtransaction ID, watching out for counter wraparound.
-	 */
-	currentSubTransactionId += 1;
-	if (currentSubTransactionId == InvalidSubTransactionId)
-	{
-		currentSubTransactionId -= 1;
-		pfree(s);
+	/* Check for overflow before incrementing the counter */
+	if (currentSubTransactionId + 1 == InvalidSubTransactionId)
 		ereport(ERROR,
 				(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
 				 errmsg("cannot have more than 2^32-1 subtransactions in a transaction")));
-	}
+
+	/* Now we can increment it without fear */
+	currentSubTransactionId += 1;
 
 	/*
 	 * We can now stack a minimally valid subtransaction without fear of

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"El que vive para el futuro es un iluso, y el que vive para el pasado,
un imbécil" (Luis Adler, "Los tripulantes de la noche")