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

Hi,

Thanks for the review.

This version is rebased over 72a6dad1c9119c5. Index stats are now
their own kind,
so the patch also carries the counts/flushed delta calculations, as index stats
can all be flushed mid-transaction.

```
+ struct
+ {
+ PgStat_IndexCounts counts;
+ PgStat_IndexCounts flushed;
+ } idx;
```

stats.sql now also tests index scans mid-transaction.

Responses inline.

> === 1
> The custom callback adds the complete pending value to shared stats but
> does not clear or baseline it:
> ...
> I think the callback should clear the published data, and that this new
> requirement should be documented as a comment. Worth adding this case to
> 001_custom_stats.pl?

Agreed. The custom callback now clears its pending counts right after adding
them to the shared entry.

```
/* Add pending counts to shared totals */
shared_entry->stats.numcalls += pending_entry->numcalls;

pgstat_unlock_entry(entry_ref);

+ /*
+ * The pending entry is retained across mid-transaction flushes (see
+ * pgstat_flush_pending_entries()), so clear what was just flushed to
+ * avoid counting it again on a later flush. Nothing reads this stat
+ * within the current transaction, so zeroing is correct.
+ */
+ pending_entry->numcalls = 0;

return PGSTAT_FLUSH_DONE;
```

I also documented the requirement in the flush_pending_cb contract, since it
applies to any variable-numbered callback.

```
+ * When xact_boundary is false the pending entry is retained, so the
+ * callback may run again for it before the transaction ends and must not
+ * merge the same pending data twice. Clear the flushed fields, or track
+ * a flushed baseline and merge only the delta. A callback must not call
+ * pgstat_force_next_flush().
```

And 001_custom_stats.pl now forces a mid-transaction flush and checks the value
is counted once.

> === 2
> I wonder if it wouldn't make more sense to subtract the converted
> cumulative values instead, something like:
> INSTR_TIME_GET_MICROSEC(localent->counts.total_time) -
> INSTR_TIME_GET_MICROSEC(localent->flushed.total_time)
> to avoid losing the fractional microseconds at each flush.

Right, converting the delta at each flush loses precision.
It now converts each cumulative value and subtracts in microseconds.

```
- total_delta = localent->counts.total_time;
- INSTR_TIME_SUBTRACT(total_delta, localent->flushed.total_time);
- shfuncent->stats.total_time += INSTR_TIME_GET_MICROSEC(total_delta);
+ shfuncent->stats.total_time +=
+ INSTR_TIME_GET_MICROSEC(localent->counts.total_time) -
+ INSTR_TIME_GET_MICROSEC(localent->flushed.total_time);
```

> === 3
> That means that a transaction touching one new entry and forcing a flush after
> each step would execute 1 + 2 + ... + N callbacks. Function entries would also
> acquire their shared entry lock again even when their delta is zero.

Right, an unchanged entry should not take its lock. The function callback now
byte-compares its counts against the flushed baseline and returns
PGSTAT_FLUSH_DONE before taking the lock, matching the relation callback.

```
+ if (memcmp(&localent->counts, &localent->flushed,
+ sizeof(PgStat_FunctionCounts)) == 0)
+ return PGSTAT_FLUSH_DONE;
```

> Do we expect the number of retained entries and forced flushes to remain small
> enough for this not to matter? Given the intended pg_stat_statements use case,
> maybe it would be worth benchmarking this before deciding that caller side
> throttling is sufficient?

I would not say the number of retained entries or forced flushes stays small,
since both are up to the caller. The point is that this is opt-in. A
process that
flushes only at transaction boundary pays nothing, since entries are deleted at
the boundary as before with no re-walk. Only a caller that forces flushes
mid-transaction pays, and that cost is a memcmp per retained entry, not a lock.

On throttling, the caller controls that on their side by forcing fewer
flushes. To
make the re-walk cheaper we would keep a separate dirty list that the
flush walks
directly, but that means marking an entry dirty on the hot counting path, which
seems like a worse trade-off to me.

For the ongoing pg_stat_statements work, it flushes when
pg_stat_statements_internal() is called, and only in the backend
reading the view,
so that backend sees its own mid-transaction changes. For now this is
to make the
regression tests work. It is not a flush forced on every backend, and the common
case is still a flush at transaction boundary, so the re-walk does not become an
issue.

The larger goal is to let a transaction that runs many statements report its
non-transactional counters, whether from core or an extension, before the
transaction ends. That is the next step, and the patch in [1] demonstrates it.

> === 4
> IIUC, if a custom flush callback calls pgstat_force_next_flush(), this
> would re enter pgstat_flush_pending_entries() ...
> Should we add a "flush in progress" protection, or document and assert
> that flush callbacks must not call pgstat_force_next_flush()?

Added the protection. pgstat_force_next_flush() now skips the immediate flush
while one is running and only sets the deferred flag.

```
+ /*
+ * When called inside a transaction, flush immediately. Skip this if a
+ * flush is already running.
+ */
- if (IsTransactionOrTransactionBlock())
+ if (!pgStatFlushInProgress && IsTransactionOrTransactionBlock())
pgstat_report_stat(true);

pgStatForceNextFlush = true;
```

pgstat_report_stat() sets pgStatFlushInProgress while it flushes and resets it
on entry, so a callback calling pgstat_force_next_flush() cannot re-enter. An
error that leaves the flag set is harmless, since pgstat_report_stat() resets
it on its next entry and a skipped force only defers the flush.

> === 5
> Maybe this paragraph should be qualified as describing the normal
> automatic flushing behavior and mention that an explicit forced flush is
> an exception?

Done. Here is the new paragraph.

```
... so a query or transaction still in progress does not affect the
displayed totals and the displayed information lags behind actual
activity, unless the process is asked to flush its pending statistics
by calling pg_stat_force_next_flush().
```

What do you think?

[1] https://www.postgresql.org/message-id/CAA5RZ0u84eMFeFWMaEo0D84ed3jF_RY0%3DRWD8tDwQWsNsU1qvA%40mail.gmail.com

--
Sami Imseih
Amazon Web Services (AWS)