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

> > Adding post_flush_pending_cb can go in as a pre-requisite
> > commit. This is also better in terms of separation of
> > responsibilities between flushing the kinds stats and post
> > flush actions.
>
> I'm not sure a new callback is needed here. The relation callback already performs
> the database accumulation only after a successful flush. If pending lifetime
> and queue membership are separated, the existing pgstat_prep_database_pending()
> call could requeue the database entry and preserve the current behavior.

For now, I moved away from the extra callback idea, although I do think it
has benefits which could be hashed out in a different thread. The relation
callback still calls pgstat_prep_database_pending().

Rather than requeue individual entries, whenever a flush_pending_cb()
accumulates to another pending entry as is the case with relation -> database,
it sets a new global pgStatPendingFlushExtra = true, which then triggers
a secondary re-scan. We continue doing secondary re-scans until
pgStatPendingFlushExtra is back to false, so this can handle arbitrary numbers
of cascading pending entries, such as the case you raise earlier. This rescan
will only occur when we are mid-transaction, as it has no purpose on transaction
boundary.

I felt this is simpler than requeueing. Because each flush is a delta against
the flushed baseline, re-flushing an unchanged entry is a no-op, so the rescan
is safe without tracking per-entry queue membership; a callback only sets
pgStatPendingFlushExtra rather than managing the pending list. The rescan does
revisit entries with nothing new, but it only runs mid-transaction, so the
normal transaction boundary flush path is unaffected.

I also added an ample amount of test coverage in stats.sql and also testing
the cascading cases in test_custom_var_stats. I do create 2 new kinds to
test the cascade, which means we need to reserve 2 new Kind IDs.
If that is a problem, we can leave these tests out perhaps?

```
+/*
+ * Kind IDs for cascade flush test (A -> B -> C).
+ * Tests that pgStatPendingFlushExtra handles multi-level dependencies.
+ */
+#define PGSTAT_KIND_CASCADE_B 27
+#define PGSTAT_KIND_CASCADE_C 28
```

> I think that a post-flush callback might improve organization, but that seems
> independent of this issue and so would need to be justified on its own.

Yes, I still think there is value here, but I will table it for another idea.

With regards to the earlier comments.

> I think that the same solution for "=== 2" would work here: keep the counters
> cumulative for the transaction, record how much has already been flushed, and
> report only the difference.

done.

> === 4

> > Static callbacks (IO, WAL, SLRU, backend, lock) don't use
> > entry_ref->pending, so there is no freeing risk.
> > They also don't
> > receive xact_boundary, so no changes were needed there.

> The concern was not freeing: as flush_static_cb can now be called inside a
> transaction, I think that it should receive xact_boundary or be skipped. That
> would allow custom stats to also defer transaction dependent state.

done

> === 5

> Should we mark pg_stat_force_next_flush() as PARALLEL UNSAFE, since the patch
> can now invoke custom flush callbacks while a parallel query is active?

You're correct.

Attached is v4.

--
Sami Imseih
Amazon Web Services (AWS)