Hi,
On Fri, Feb 13, 2026 at 08:23:01PM -0600, Sami Imseih wrote:
> > PFA attached v6, addressing the reviews comments.
>
> v6 is getting closer IMO. Here are some comments I have.
Thanks!
> v6-0001 looks solid, but some minor comments:
> 1/
>
> + pgstat_flush_backend(nowait, PGSTAT_BACKEND_FLUSH_WAL, true);
>
> let's also use explicit (void) cast here
Yeah. The patch did not introduce this inconsistency but let's fix it in passing.
> 2/
>
> + * Timeout handler for flushing non-transactional stats.
>
> I also noticed in v6-0005, we refer to "anytime" stats as
> "non-transactional". It's better to just refer to them as "anytime"
> anywhere instead of "non-transactional:.
Done.
> v6-0002:
>
> The logic here should not try to acquire the lock twice.
Yeah, changed to match the pgstat_wal_flush_cb() logic.
> + -- Force anytime flush (inside transaction!)
> + select pg_stat_force_anytime_flush();
>
> Not sure why we need pg_stat_force_anytime_flush.
> A pg_sleep is sufficient, like below. right?
Right, done.
>
> v6-0003:
>
> 1/
> Suggested doc changes:
Done.
> 2/
> I don't see we have tests for other timeout based GUCs, but it would nice
> to ensure that this woks correctly. Maybe as a custom_stats test where we
> SET stats_flush_interval inside the transaction and make sure the stats flush
> only after the new timeout has passed. Maybe?
Not sure I follow, that's what 0002 is doing.
> v6-0004:
>
> 1/
>
> NIT:
>
> +$node_primary->append_conf('postgresql.conf', "stats_flush_interval= '1s'");
>
> +$node_publisher->append_conf('postgresql.conf', "stats_flush_interval= '1s'");
>
> missing space before the equal sign.
Done.
>
> v6-0005:
>
> 1/
>
> /* Partial flush only happens in anytime mode for FLUSH_ANYTIME stats */
> is_partial_flush = (anytime_only && kind_info->flush_mode ==
> FLUSH_ANYTIME);
>
> Will this be tue at all time? Let's imagine a Kind that flushes all the fields
> ANYTIME, would we not want to delete the pending entry?
>
> + /* if successfull non-partial flush, remove entry */
> + if (did_flush && !is_partial_flush)
> pgstat_delete_pending_entry(entry_ref);
>
Right, and that's why the MIXED flush mode was useful, i.e to be able to distinguish
here. So, in the attached, instead of re-introducing the MIXED flush mode, I added
a "is_partial" bool paramater to the flush_pending_cb().
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
