Re: pgstat: Flush some statistics within running transactions, take 2

Hi,

I posted v7, which addresses both Bertrand's earlier points and the
discussion about splitting the table stats counters into transactional
and non-transactional v7-0001 does the refactoring to split the table
stats and v7-0002 is now the in-transaction flush change.

I also expanded the custom stats example and TAP coverage in v7-0002
to show the same transactional and non-transactional split for
extension stats too.

> I wonder if pgStatFlushInProgress should be reset through PG_FINALLY?
> The reset at the beginning of pgstat_report_stat() could then become
> an assert.

Done in v7. pgstat_report_stat() now uses PG_TRY/PG_FINALLY to clear
pgStatFlushInProgress on all exit paths, and the reset at entry became
an assert. Initially, I thought that pgStatFlushInProgress would just
correct itself on the next flush, but this also means that
in-transaction, if there are consecutive flushes and the first one
errors out, the second one will not flush anything, but the third one
will, which is probably ok in practice, but not ideal.

> After a HOT or new-page update, every subsequent in transaction flush
> takes the relation lock, even when there is nothing new to flush
> because those deferred counters keep the memcmp() unequal.
>
> I wonder if, when flush_txn is false, we should check whether any of
> the non transactional counters changed before taking the lock? If only
> deferred counters differ, the callback could return
> PGSTAT_FLUSH_PARTIAL immediately.

Done in v7. The relation flush path now compares only the
non-transactional group before taking the lock for an in-transaction
flush. If there are no changes in the non-transactional stats, it
returns PGSTAT_FLUSH_PARTIAL immediately.

I manually verified this fix because adding a test will require
an injection point, which I don't think is worthwhile.

> Some comments look stale
>
> "Once the stats are flushed, PgStat_EntryRef->pending is freed."
>
> The pending entry can now be retained until the transaction boundary.
>
> "/* Force statistics to be reported at the next occasion */"
>
> It can flush immediately.

Done in v7. I updated those comments to match the new behavior.

> s/PgStat_TableStatus/PgStat_RelationStatus/?

Done in v7.

> "pg_stat_force_next_flush() is not documented"
>
> s/is not/is?

Done in v7.

--
Sami Imseih
Amazon Web Services (AWS)