Hi, On Thu, Apr 03, 2025 at 09:25:02AM +0530, vignesh C wrote: > On Mon, 31 Mar 2025 at 18:35, Bertrand Drouvot > <bertranddrouvot...@gmail.com> wrote: > > > Couple of suggestions:
Thanks for looking at it! > 1) I felt we can include a similar verification for one of the logical > replication tests too: > +# Wait for the walsender to update its IO statistics. > > +# Has to be done before the next restart and far enough from the > +# pg_stat_reset_shared('io') to minimize the risk of polling for too long. > +$node_primary->poll_query_until( > + 'postgres', > + qq[SELECT sum(reads) > 0 > + FROM pg_catalog.pg_stat_io > + WHERE backend_type = 'walsender' > + AND object = 'wal'] > + ) > + or die > + "Timed out while waiting for the walsender to update its IO statistics"; > + Initially ([1]) it was added in 035_standby_logical_decoding.pl. But this test (035) is already racy enough that I felt better to move it to 001_stream_rep.pl (see [2]) instead. I don't think that's a big issue as the code path being changed in the patch is shared between logical and physical walsenders. That said that would not hurt to add a logical walsender test, but I would prefer it to be outside 035_standby_logical_decoding.pl. Do you have a suggestion for the location of such a test? > 2) We can comment this in a single line itself: > + /* > + * Report IO statistics > + */ Right, but: - it's already done that way in walsender.c (see "Try to flush any pending output to the client" for example). - the exact same comment is written that way in pgstat_bgwriter.c and pgstat_checkpointer.c So that I just copy/paste it. [1]: https://www.postgresql.org/message-id/Z73IsKBceoVd4t55%40ip-10-97-1-34.eu-west-3.compute.internal [2]: https://www.postgresql.org/message-id/Z77jgvhwOu9S0a5r%40ip-10-97-1-34.eu-west-3.compute.internal Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com