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)
