From: =?utf-8?q?=D0=94=D0=B0=D0=B2=D1=8B=D0=B4=D0=BE=D0=B2_=D0=92=D0=B8=D1=82=D0=B0=D0=BB=D0=B8=D0=B9?= <v.davydov@postgrespro.ru>
In-Reply-To: <Z2zq-GrOpIDPOHUj@paquier.xyz>
Content-Type: text/plain; charset="utf-8"
References: <13b5b6-676c3080-4d-531db900@47931709>
 <Z2yKWtB4iB8BdEiI@paquier.xyz> <Z2zq-GrOpIDPOHUj@paquier.xyz>
Date: Thu, 26 Dec 2024 18:11:25 +0300
Cc: "pgsql-hackers" <pgsql-hackers@postgresql.org>
To: "Michael Paquier" <michael@paquier.xyz>
MIME-Version: 1.0
Message-ID: <152dda-676d7200-61-659acf0@102179597>
Subject: =?utf-8?q?Re=3A?= An improvement of ProcessTwoPhaseBuffer logic
User-Agent: SOGoMail 5.11.2
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/152dda-676d7200-61-659acf0%40102179597>
Precedence: bulk

Hi Michael,

> Here are two patches to address both issues:

Thank you for the preparing the patches and test simplification. My bad=
, I overcompilcated it.

I concerned about twophase file name generation. The function TwoPhaseF=
ilePath() is pretty straitforward and unambiguous in 16 and earlier ver=
sions. The logic of this function in 17+ seems to be more complex. I do=
 not understand it clearly. But, I guess, it will work incorrectly afte=
r turning to a newer epoch, because the epoch is calculated from Transa=
mVariables->nextXid, but not from problem xid. The same problem may hap=
pen if we are in epoch 1 or greater. It will produce a wrong file name,=
 because the epoch will be obtained from the last xid, not from file na=
me xid. In another words, the function AdjustToFullTransactionId assume=
s that if xid > TransamVariables->nextXid, then the xid from the previo=
us epoch. I may be not the case in our scenario.

> I suspect that this can be still dangerous as-is while complicating t=
he code with more possible paths for the removal of the 2PC files
Agree, but we may pass file name into TwoPhaseFilePath if any, instead =
of creation of two functions as in the patch. The cost - two new if con=
ditions. Using file names is pretty safe. Once we read the file and ext=
ract xid from its name, just pass this file name to TwoPhaseFilePath().=
 If not, try to generate it. Anyway, I do not insist on it, just try to=
 discuss.

With best regards,
Vitaly