Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 13 Dec 2023 15:13:20 +0100
From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
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: <ZXm8AGdeOR5feHDo@paquier.xyz>
References: <ZXggXVwX6F6gWZDq@paquier.xyz>
 <202312121711.tzjyb5yeb3fa@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="LRjFRBuVI37RIk+2"
Content-Disposition: inline
In-Reply-To: <202312121711.tzjyb5yeb3fa@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/ZXm8AGdeOR5feHDo%40paquier.xyz>
Precedence: bulk


--LRjFRBuVI37RIk+2
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Dec 12, 2023 at 06:11:44PM +0100, Alvaro Herrera wrote:
> 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.

Yep.

> 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.

AFAIK, this only impacts the visibility of the data retrieved in
transactions began when recovery was still running when doing an index
scan, and, even if the HOT chains would get inconsistent, we'd still
clean up them one the xmin gets updated on the standby because there
are no snapshots requiring it.  REINDEX after promotion does not looks
like an issue to me as we'd take a lock or wait for the necessary
snapshots to be gone in the concurrent case.  Or perhaps my
imagination with the HOT chains is too limited?

> 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:

That crossed my mind, but it is not like we have other code paths
where we assume some fields to be initialized based on a palloc0() or
similar, so I don't see why we'd need that for this case.

> 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:

Saying that, agreed that this suggestion is an improvement compared to
what's on HEAD.  So +1 from me to change this code as you are
suggesting.
--
Michael

--LRjFRBuVI37RIk+2
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAEBCAAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmV5u/8ACgkQnvQgOdby
QH13dQ//S81s793sETry9nlKflwqiJq9WOAnaWzAB3zZaFe8SdLsMbiwsXZpy+BY
Ov/7lLzn+JjMDV7ZcIL+MoNKUFMXa6GrfFg+mLg2C+6+pu8fAIe+I6mzn19me5z8
psZ3QoiywKMP4nCks/NpwJd+D2/HprcsO6qUoSpRFUhB3bEGAW0gKWfpb6HfQn4L
6ieeTjTzfhJ3SmNUDmq/fo/HDWoxKqm8uIsgoj40BMWVyM4Y4wd+RhRLti09USA0
UFa+dreXRPmlq5YNo1nEtgQkHwq3wgswS7yD5+bMmj3ZJoXI1nMfQyRMTaLkjgcg
FQMLq0h0iZF6Asuag1HhwbJLQuznaawKbZFkT993u/uTuuzVGGzOejMRAx9QmjSr
k7/uNmSHGdHSMyPutDyacW8gjTWSRuF/2c7sP+NQ4u5kG9FEo65M0DtfhASIYgA7
nu8pgfLYCxFgO+H6TlPqyi5llHtDBoz0OGkJ5jNX4eHgfMWHAlsmuzPqabZEbs6Y
w2XHKI6DJvMIfUWZQclSrwbi2Z/JzUySiDY6BE+goAczIoqrBibP29VbN4+w9Ns1
S994X42q97s8kpGLiApBJDw61q/AEsP7BbgkA1g8toreEbVRksTfogufLvYiEwQY
+6f3f8mviahjGbHYoAPmRkFciJ7W91Mny0ZBFA+LiOjgV/9MBfQ=
=AS2V
-----END PGP SIGNATURE-----

--LRjFRBuVI37RIk+2--