Re: Improve pg_stat_statements scalability

> > 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