| From: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
|---|---|
| To: | Sami Imseih <samimseih(at)gmail(dot)com> |
| Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> |
| Subject: | Re: Flush some statistics within running transactions |
| Date: | 2026-02-16 08:27:56 |
| Message-ID: | aZLVDJLTiFlxsWZi@ip-10-97-1-34.eu-west-3.compute.internal |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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
| Attachment | Content-Type | Size |
|---|---|---|
| v7-0001-Add-pgstat_report_anytime_stat-for-periodic-stats.patch | text/x-diff | 41.9 KB |
| v7-0002-Add-anytime-flush-tests-for-custom-stats.patch | text/x-diff | 9.0 KB |
| v7-0003-Add-GUC-to-specify-non-transactional-statistics-f.patch | text/x-diff | 8.9 KB |
| v7-0004-Remove-useless-calls-to-flush-some-stats.patch | text/x-diff | 7.7 KB |
| v7-0005-Change-RELATION-and-DATABASE-stats-to-anytime-flu.patch | text/x-diff | 34.2 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Hayato Kuroda (Fujitsu) | 2026-02-16 08:33:47 | RE: Improve docs syntax checking and enable it in the meson build |
| Previous Message | Hunaid Sohail | 2026-02-16 08:20:58 | Re: Proposal: SELECT * EXCLUDE (...) command |