> > After further thought, I don't think passing a context flag to the > > callback is necessary here.
> I think this is needed so that custom stats extensions know that it exists (as > they would get a compilation errors and would need to make use of it). > Indeed, if we don't add a flag then it's easy for a custom stats kind to > crash on assert-enabled build or produces weird behavior on non-assert enabled > build. All it has to do is, for example, call > GetCurrentTransactionStopTimestamp() > at flush time, say: I think you are suggesting that the context is added to solidify a contract on how the callback should be used. I am not against that idea for the clarity of the API, but it does not overcome developer errors. > * Ignore entries that didn't accumulate any actual counts, such as > - * indexes that were opened by the planner but not used. > + * indexes that were opened by the planner but not used. With > + * in-transaction flushing an entry may be flushed multiple times, so > keep > + * it pending if it has active transaction state and commit will merge > + * counters into it. > */ > if (pg_memory_is_all_zeros(&lstats->counts, > sizeof(struct > PgStat_TableCounts))) > - return true; > + return (lstats->trans == NULL); > > One of my previous comments was that this change makes the callback > return value carry two different meanings. Both mean that the pending list did not completely flush, so you must try to flush it later; either because we could not acquire the lock ( existing reason ) or because we only flushed non-transactional stats ( the new reason ). Either way, the handling does not change regardless of the outcome, it means that we need to flush again. right? > Since whether lstats->trans > is NULL is purely part of the calling context and not derived from the > callback's own work, I still think it is cleaner to keep > pgstat_relation_flush_cb() unchanged and let the caller handle that > distinction instead. I am confused about this. How would we keep pgstat_relation_flush_cb() unchanged? The callback is ultimately responsible for knowing what to flush depending on the state ( in transaction --or-- end of transaction ) -- Sami
