Re: Improve pg_stat_statements scalability

> I agree with that part. The callback certainly needs to know what can
> be flushed in the current context.
>
> What I was trying to separate is the callback's flush logic from the
> meaning of its return value.
>
> Previously, the callback's return value indicated whether the requested
> flush completed, for example, whether it failed to acquire a lock. With
> this change, the same value would also indicate that some state was
> intentionally excluded from an in-transaction flush. Although both
> cases leave the entry pending for a later flush, they represent
> different outcomes.
>
> The callback still needs to determine what can be flushed and whether
> anything remains. My point is only that intentionally retaining
> transactional state due to the flush context should not be represented
> as a failure of the callback's own work. I think it would be cleaner
> for the caller to combine the callback's result with the flush context
> when deciding whether to remove the entry from the pending list.
>
> Similarly, the context flag is not intended to prevent every developer
> mistake. Rather, it makes the changed callback contract explicit, so
> extensions implementing custom stats kinds are forced to revisit their
> callbacks instead of silently assuming the previous semantics.

I think there are 2 different asks, so let me see if my understanding
is correct:

1/ Explicit callback contract:

Add a "bool xact_boundary" which tells the callback whether it is
safe to flush transactional counters. When true (transaction end),
the callback flushes everything. When false (mid-transaction), the
callback flushes only non-transactional counters unless the entry
has no active transactional state. So the new _cb will be:

```
bool (*flush_pending_cb) (PgStat_EntryRef *sr, bool nowait,
bool xact_boundary);
```

Currently, there is only one caller now which is
pgstat_flush_pending_entries() and it will
call the _cb like this:

```
flush_result = kind_info->flush_pending_cb(entry_ref, nowait,
!IsTransactionOrTransactionBlock());
```

But in theory this _cb can be invoked by other callers.

Inside pgstat_relation_flush_cb(), the following will drive if we should flush
in-transaction safe counters or not:

```
flush_txn = (xact_boundary || lstats->trans == NULL);
```

2/ The return value of the callbacks is now an enum rather than a bool.

Previously it returned true (flush completed) or false (could not complete).
The issue raised with my earlier proposal is that "false" carried two different
meanings; lock contention or deferred transactional boundary stats.

The new enum makes the result explicit:

```
PGSTAT_FLUSH_DONE - fully flushed, remove from pending list
PGSTAT_FLUSH_LOCK_CONFLICT - could not acquire lock (nowait)
PGSTAT_FLUSH_PARTIAL - partial flush, transaction boundary stats deferred
```

As it stands now, FLUSH_LOCK_CONFLICT and FLUSH_PARTIAL require the same
action (keep pending, retry later), but at least it is explicit why we
are retrying. This also forces extension authors to choose the correct
return value rather than blindly returning true/false.

Does this match what you had in mind?

--
Sami Imseih
Amazon Web Services (AWS)