Re: Custom pgstat support performance regression for simple queries

Hi,

On Fri, Jul 25, 2025 at 10:38:59AM +0900, Michael Paquier wrote:
> On Thu, Jul 24, 2025 at 07:38:46AM +0000, Bertrand Drouvot wrote:
> > What about?
> >
> > - create a global variable but that should be used only by extensions? Say,
> > pgstat_report_custom_fixed
> >
> > - for builtin fixed-sized, just rely on have_iostats, have_slrustats and friends
> >
> > Then in pgstat_report_stat():
> >
> > - do the check on have_iostats, have_slrustats and friends and on
> > pgstat_report_custom_fixed
> >
> > - reset pgstat_report_custom_fixed
> >
> > That way I think it's less error prone for the builtin stats and more clear
> > for the extensions (as at least "custom" is also part of the global variable
> > name and the check in pgstat_report_stat() would make it clear that we rely on
> > the custom global variable).
> >
> > Thoughts?
>
> FWIW, I see more benefits in the simplicity of a single flag to govern
> both builtin and custom kinds, but I can see that this may come down
> to personal taste. A single flag is simpler here, and cheap.
>
> What have_static_pending_cb was originally aiming for is the removal
> of any knowledge specific to stats kinds in the central pgstats APIs,
> which is why the flags specific to IO, SLRU and WAL have been moved
> out into their own files.

Yes, with my idea we'd need to move them back.

> Letting custom stats manipulate pgstat_report_fixed or invent a new
> pgstat_report_custom_fixed would be logically the same thing, but
> this comes down to the fact that we still want to decide if a report
> should be triggered if any of the fixed-numbered stats want to let
> pgstat_report_stat() do one round of pending stats flush.

Yes.

> Having a second flag would keep this abstraction level intact.

Not a second, just one global "pgstat_report_custom_fixed" and the specifics
flags for IO, SLRU, something like:

if (dlist_is_empty(&pgStatPending) &&
!have_iostats &&
!have_slrustats &&
!pgstat_report_custom_fixed &&
!pgstat_have_pending_wal())

> Now
> that leads to overcomplicating the API

Not sure.

> for a small gain in
> debuggability to know which part of the subsystem wants the report to
> happen at early stages of pgstat_report_stat(). If we want to know
> exactly which stats kind wants the flush to happen, it may be better
> to reuse your idea of one single uint32 or uint64 with one bit
> allocated for each stats kind to have this information available in
> pgstat_report_fixed(). However, we already know that with the
> flush_static_cb callback, as dedicated paths can be taken for each
> stats kinds. Once again, this would require stats kind to set their
> bit to force a round of reports, gaining more information before we
> try to call each flush_static_cb.

Yeah. I'm fine with one single flag as you proposed, I just have the feeling
it's more error prone.

As an example:

@@ -1091,6 +1092,9 @@ XLogInsertRecord(XLogRecData *rdata,
pgWalUsage.wal_bytes += rechdr->xl_tot_len;
pgWalUsage.wal_records++;
pgWalUsage.wal_fpi += num_fpi;
+
+ /* Required for the flush of pending stats WAL data */
+ pgstat_report_fixed = true;
}

return EndPos;
@@ -2108,6 +2112,12 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
LWLockRelease(WALWriteLock);
pgWalUsage.wal_buffers_full++;
TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DIRTY_DONE();
+
+ /*
+ * Required for the flush of pending stats WAL data, per
+ * update of pgWalUsage.
+ */
+ pgstat_report_fixed = true;
}

This was not needed before fc415edf8ca8, and I think it was better to use
pgstat_have_pending_wal() to not have to set a flag to every places pgWalUsage.XX
changes.

Having said that, I think the single global flag patch is pretty straightforward
and LGTM.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com