Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 26 Dec 2024 07:42:34 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Vitaly Davydov <v.davydov@postgrespro.ru>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: An improvement of ProcessTwoPhaseBuffer logic
Message-ID: <Z2yKWtB4iB8BdEiI@paquier.xyz>
References: <13b5b6-676c3080-4d-531db900@47931709>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="77Lf7YK1DkvgBR66"
Content-Disposition: inline
In-Reply-To: <13b5b6-676c3080-4d-531db900@47931709>
Archived-At: <https://www.postgresql.org/message-id/Z2yKWtB4iB8BdEiI%40paquier.xyz>
Precedence: bulk


--77Lf7YK1DkvgBR66
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Dec 25, 2024 at 07:18:18PM +0300, Vitaly Davydov wrote:
> I tried your patch and it seems the server is started
> successfully. But I've found another problem in my synthetic test -
> it can not remove the file with the following message:
>=20
> LOG:  database system was not properly shut down; automatic recovery in p=
rogress
> WARNING:  removing future two-phase state file for transaction 1045224
> WARNING:  could not remove file "pg_twophase/FFFFFFFF000FF2E8": No such f=
ile or directory
> LOG:  redo starts at 0/1762978

That works properly up to v16~.  I have slept over this problem and I
think that we'd better backpatch the fix for the first problem.  Now,
I also want to add a test to provide coverage.  As you say, that's
simple:
- Create an empty file with a future XID name
- Restart the server.
- Scan the logs for the WARNING where the future file is removed.

> The fill will never be removed automatically.
> I guess, it is because we incorrectly calculate the two-phase file
> name using TwoPhaseFilePath in RemoveTwoPhaseFile in this
> scenario. It can be fixed if to pass file path directly from
> RecoverPreparedTransactions or StandbyRecoverPreparedTransaction
> into ProcessTwoPhaseBuffer -> RemoveTwoPhaseFile.

Yeah, I've noticed that as well.  We are dealing with a second bug
that affects 17~ caused by the switch to FullTransactionIds for 2PC
file names, which had been introduced in 5a1dfde8334b (pretty sure
that it is the culprit, did not bisect).  Attempting to remove a
future file triggers an assertion failure:
#3  0x00007fed046324f0 in __GI_abort () at ./stdlib/abort.c:79
#4  0x0000563e68a9bd07 in ExceptionalCondition
(conditionName=3D0x563e68c66fc3 "epoch > 0", fileName=3D0x563e68c66b0b
"twophase.c", lineNumber=3D953) at assert.c:66
#5  0x0000563e67683060 in AdjustToFullTransactionId (xid=3D4095) at
twophase.c:953
#6  0x0000563e67683092 in TwoPhaseFilePath (path=3D0x7ffcb515b4a0
"h\307!\227>V", xid=3D4095) at twophase.c:963
#7  0x0000563e67686603 in RemoveTwoPhaseFile (xid=3D4095,
giveWarning=3Dtrue) at twophase.c:1728
#8  0x0000563e67688989 in ProcessTwoPhaseBuffer (xid=3D4095,
prepare_start_lsn=3D0, fromdisk=3Dtrue, setParent=3Dfalse, setNextXid=3Dfal=
se)
at twophase.c:2219
#9  0x0000563e676872a6 in restoreTwoPhaseData () at twophase.c:1924
#10 0x0000563e676b5c8b in StartupXLOG () at xlog.c:5642=20

So this means that we are dealing with two different bugs, and that we
need to fix the assertion failure of the second problem first down to
17, then fix the first problem on all stable branches with a test to
cover both the first and second problems.

> I did it in the proposed patch
> https://www.postgresql.org/message-id/cedbe-65e0c000-1-6db17700%401332698=
62
> (it is incomplete now).

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,
because we may still build a file name from an XID, and that's what's
causing this issue..
--
Michael

--77Lf7YK1DkvgBR66
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmdsiloACgkQnvQgOdby
QH0lzA//TKRvh5hjYdJXHhXQZcsFmZZB7kMUgqz3tg3r9Q9XGKdyW0ZLSp9dYb3V
0TTvrWv3SLD6Hd876hWXBR0G7ky6yWJbJ/u/4j4WA/LYr8fG4QftsnSEL2NrovbP
V1/bB3VX7+CK8Pt7KRdB1mKbqx+NBf1v1RAQpXA1FkV7UJ/dpPjCzuBCV8u+Xgin
08bRDcrRfdqnp7ZBQ0HmF4oGYFklR1nrjyjTH0YMEqP2r1eVGPDb0p354I3XkdWK
6icURvopHDnnE/yWC/YSt+j1Wi8nhcBsNzfkPb9HksDk5X6Ws9U0sit8gAwVxGpJ
qkpavkrKrJsDOVLnJaqozioEjUBu7Wqfr7JyUShimBkYZf5VNK8DsmdUnm8mDGK0
QM+QEzXJEdFF68YzuMDZe1g8BxubSVxGvduol8VqyfnTeevUXYGBDTR0bqw5WFvP
vZPRfA7hkfed46VbTYCVrIbJ521qXmZr5ss03ohu4wtyhkWCE3qopI7lhaaPU0uD
TuFoZ+D4YPGR6YzqsTZcPhG3DxEd5YwOq3/z7y0mQK2HAg/Xew3dIImuwvlL4Lqe
BYiFn6kgWAeGEpCOJ9/D1wO3DREPmJIGBQMsYGDPocKBehzTzGh5dexYcV0h1EQi
ntACBnmMrrWSF6neTPYM/UyvuHnG2zLDqSbLAKwTaUDPn6uvw3U=
=zzWq
-----END PGP SIGNATURE-----

--77Lf7YK1DkvgBR66--