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

Thanks, Bertrand!

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

Agreed. My thinking was that a stale flag would self-heal, at the cost of
blocking only the next immediate in-transaction flush while the rest keeps
working. That is probably OK but not ideal, so I will add the
PG_TRY/PG_FINALLY and turn the reset at the start of pgstat_report_stat()
into an assert.

> === 2 ===
> + if (memcmp(&lstats->tab.counts, &lstats->tab.flushed,
> + sizeof(struct PgStat_TableCounts)) == 0)
> + return flush_txn ? PGSTAT_FLUSH_DONE : PGSTAT_FLUSH_PARTIAL;
>
> 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.

Good catch. Likely not a big deal in practice, but it points at something
more fundamental. PgStat_TableCounts mixes transactional and non-transactional
counters in no particular order, so a memcmp() of the whole struct cannot tell
which group changed.

I think we should keep a single struct but split it into two contiguous
regions, non-transactional first and transactional after, with comments
marking the boundary, and use an offset to compare each group on its own.
When flush_txn is false we then compare only the non-transactional group
before taking the lock, and return PGSTAT_FLUSH_PARTIAL when only the
deferred counters differ.

I would pull this out as a pre-req patch ahead of the main change. The
custom stats module could show the same pattern so extension developers are
mindful of this.

What do you think? I would also like to hear Michael's view on this before
I post the next revision.

--
Sami Imseih
Amazon Web Services (AWS)