Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 22 Jan 2024 15:54:44 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>,
	"Yu Shi (Fujitsu)" <shiy.fnst@fujitsu.com>,
	Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Thomas Munro <thomas.munro@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Test slots invalidations in 035_standby_logical_decoding.pl only
 if dead rows are removed
Message-ID: <Za4RNBnnpiMk8SHP@paquier.xyz>
References: <ZaACEPtVovKlRVUf@ip-10-97-1-34.eu-west-3.compute.internal>
 <6f85667e-5754-5d35-dbf1-c83fe08c1e48@gmail.com>
 <ZaBlU5FqOeDqyHtJ@paquier.xyz>
 <ZaDnIMTbT+0yR5LZ@ip-10-97-1-34.eu-west-3.compute.internal>
 <cc7925b8-30cc-c76d-b1b6-c9ec6bd36a03@gmail.com>
 <ZaFCoJ1FxBQ9jQkX@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZaSthTtVmvKKuGNb@paquier.xyz>
 <b641be2a-945b-999c-2828-7bbd8a16a009@gmail.com>
 <d975bb14-2c90-25bf-7d71-7ce5f9d7fa82@gmail.com>
 <Zao6xfSQUry5Fkdn@ip-10-97-1-34.eu-west-3.compute.internal>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="4TKXCctTLJrFnFGb"
Content-Disposition: inline
In-Reply-To: <Zao6xfSQUry5Fkdn@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: 
 <https://www.postgresql.org/message-id/Za4RNBnnpiMk8SHP%40paquier.xyz>
Precedence: bulk


--4TKXCctTLJrFnFGb
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Jan 19, 2024 at 09:03:01AM +0000, Bertrand Drouvot wrote:
> On Fri, Jan 19, 2024 at 09:00:01AM +0300, Alexander Lakhin wrote:
>> 15.01.2024 12:00, Alexander Lakhin wrote:
>>> If this approach looks promising to you, maybe we could add a submodule=
 to
>>> perl/PostgreSQL/Test/ and use this functionality in other tests (e.g., =
in
>>> 019_replslot_limit) as well.
>>>=20
>>> Personally I think that having such a functionality for using in tests
>>> might be useful not only to avoid some "problematic" behaviour but also=
 to
>>> test the opposite cases.
>>=20
>> After spending a few days on it, I've discovered two more issues:
>> https://www.postgresql.org/message-id/16d6d9cc-f97d-0b34-be65-425183ed37=
21%40gmail.com
>> https://www.postgresql.org/message-id/b0102688-6d6c-c86a-db79-e0e91d245b=
1a%40gmail.com
>>=20
>> (The latter is not related to bgwriter directly, but it was discovered
>> thanks to the RUNNING_XACTS record flew in WAL in a lucky moment.)
>>=20
>> So it becomes clear that the 035 test is not the only one, which might
>> suffer from bgwriter's activity,
>=20
> Yeah... thanks for sharing!
>=20
>> and inventing a way to stop bgwriter/
>> control it is a different subject, getting out of scope of the current
>> issue.
>=20
> Agree.
>=20
>> 15.01.2024 11:49, Bertrand Drouvot wrote:
>> Maybe it would be a right move to commit the fix, and then think about
>> more rare failures.
>=20
> +1

Yeah, agreed to make things more stable before making them fancier.

>> 2) Shall we align the 035_standby_logical_decoding with
>> 031_recovery_conflict in regard to improving stability of vacuum?
>=20
> Yeah, I think that could make sense.

Probably.  That can always be done as a separate change, after a few
runs of the slow buildfarm members able to reproduce the failure.

>> I see the following options for this:
>> a) use wait_until_vacuum_can_remove() and autovacuum =3D off in both tes=
ts;
>> b) use FREEZE and autovacuum =3D off in both tests;
>> c) use wait_until_vacuum_can_remove() in 035, FREEZE in 031, and
>> =A0autovacuum =3D off in both.
>=20
> I'd vote for a) as I've the feeling it's "easier" to understand (and I'm =
not
> sure using FREEZE would give full "stabilization predictability", at leas=
t for
> 035_standby_logical_decoding.pl). That said I did not test what the outco=
me would
> be for 031_recovery_conflict.pl by making use of a).

Yeah, I think I agree here with a), as v7 does for 035.

+# Launch $sql and wait for a new snapshot that has a newer horizon before
+# doing the vacuum with $vac_option on $to_vac.
+sub wait_until_vacuum_can_remove

This had better document what the arguments of this routine are,
because that's really unclear.  $to_vac is the relation that will be
vacuumed, for one.

Also, wouldn't it be better to document in the test why
txid_current_snapshot() is chosen?  Contrary to txid_current(), it
does not generate a Transaction/COMMIT to make the test more
predictible, something you have mentioned upthread, and implied in the
test.

-  INSERT INTO flush_wal DEFAULT VALUES; -- see create table flush_wal

This removes two INSERTs on flush_wal and refactors the code to do the
INSERT in wait_until_vacuum_can_remove(), using a SQL comment to
document a reference about the reason why an INSERT is used.  Couldn't
you just use a normal comment here?

>> I've re-tested the v6 patch today and confirmed that it makes the test
>> more stable. I ran (with bgwriter_delay =3D 10000 in temp.config) 20 tes=
ts in
>> parallel and got failures ('inactiveslot slot invalidation is logged with
>> vacuum on pg_authid') on iterations 2, 6, 6 with no patch applied.
>> (With unlimited CPU, when average test duration is around 70 seconds.)
>>=20
>> But with v6 applied, 60 iterations succeeded.
>=20
> Nice! Thanks for the testing!

I need to review what you have more thoroughly, but is it OK to assume
that both of you are happy with the latest version of the patch in
terms of stability gained?  That's still not the full picture, still a
good step towards that.
--
Michael

--4TKXCctTLJrFnFGb
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmWuETQACgkQnvQgOdby
QH3dDhAAllml9yfdNmFrjyjkCig1MsShD3Dp1DxDIjb4+sefPAiaPFo+TyaLzki6
UlZm5K1j/Hhq5jx3UPYs9weCuSWkIXBgnDWH0WDmUqYKrqrRvezYhGCAISIyjoEA
lixuspwP+qJFsuFRWKXvLhu1Fkfs4ZsQjkMdeF0mNFBq9DRlPS3hDSMV5uSkeF1Y
En9mInsZSgufdzxvWkuksln1DrXbq2jvDcw+mAF6WyyUP+PEeBXTN1f+5ygJdhEM
kYnXFR+zopWnV9sLOpG5NU1kXvWKcaEILzCD8ATwKYXeGmg3tN1PjqElgeCwNXQk
6gRDIElLSkNld45IM4nTnM1EPw4XUCQ9T2ykDuuyZa4lc9UJOkI9fQUi/YbXY+xK
AbtTyzrdAhaFcYX72PcEa9sZ09VjOvDAORgoVhosiHdQeiS6hSX/YM5NfiBb7zhY
HNtjcgDYwxzT9t87Kp8U6TFUluE298qIpskZPY+4WmiOKTtUHTJV504Cc17BaWC2
mcq+WlldFWXGTlm8Tvl7fSjXmWWGDb0w16bZQQcHCOr2sXgiFMUERVr/lqQD9Ng9
pkpofcika7q6jpKmx5Njz+s8NE0tYyLTWkbGA0CFtH4OvP0EVEorpf9IOieRBUxg
Q3B11xUYykTGXopwL2e8jxfzNeObRQL2T1wpYd3jQqLxMai3/Lo=
=OA2f
-----END PGP SIGNATURE-----

--4TKXCctTLJrFnFGb--