Horton

Re: An improvement of ProcessTwoPhaseBuffer logic

Vitaly Davydov <v.davydov@postgrespro.ru>

View thread
From: Давыдов Виталий <v.davydov@postgrespro.ru>
To: "Michael Paquier" <michael@paquier.xyz>
Cc: "pgsql-hackers" <pgsql-hackers@postgresql.org>
Date: 2024-12-26T15:11:25Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Integrate FullTransactionIds deeper into two-phase code

  2. Merge copies of converting an XID to a FullTransactionId.

  3. Revert recent changes related to handling of 2PC files at recovery

  4. Fix failures with incorrect epoch handling for 2PC files at recovery

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 TwoPhaseFilePath() is pretty straitforward and unambiguous in 16 and earlier versions. 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 after turning to a newer epoch, because the epoch is calculated from TransamVariables->nextXid, but not from problem xid. The same problem may happen 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 name xid. In another words, the function AdjustToFullTransactionId assumes that if xid > TransamVariables->nextXid, then the xid from the previous epoch. I may be not the case in our scenario.

> I suspect that this can be still dangerous as-is while complicating the 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 conditions. Using file names is pretty safe. Once we read the file and extract 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