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

Thanks for the findings!

> Thanks for the new patch version!
>
> Some comments:
>
> === 1
>
> > I moved the protection into
> > pgstat_flush_pending_entries() itself, so all callbacks are protected.
>
> > if (result == PGSTAT_FLUSH_DONE && xact_boundary)
> > pgstat_delete_pending_entry(entry_ref);
> > else
> > have_pending = true;
>
> That produces a corner case for database and relation stats, for example:
>
> "
> CREATE TABLE flush_db_lag AS
> SELECT i FROM generate_series(1, 100) AS g(i);
>
> SELECT pg_stat_force_next_flush();
>
> BEGIN;
> SET LOCAL stats_fetch_consistency = none;
> SELECT 1 FROM pg_class LIMIT 1;
> SELECT pg_stat_force_next_flush();
> SELECT pg_stat_reset();
> SELECT pg_stat_get_db_tuples_returned(5) AS db_before,
> pg_stat_get_tuples_returned(16384) AS rel_before;
>
> SELECT count(*) FROM flush_db_lag;
> SELECT pg_stat_force_next_flush();
>
> SELECT pg_stat_get_db_tuples_returned(5) AS db_after_first,
> pg_stat_get_tuples_returned(16384) AS rel_after_first;
>
> SELECT pg_stat_force_next_flush();
>
> SELECT pg_stat_get_db_tuples_returned(5) AS db_after_second,
> pg_stat_get_tuples_returned(16384) AS rel_after_second;
> "
>
> Produces:
>
> db_after_first | rel_after_first
> ----------------+-----------------
> 11 | 100
>
> db_after_second | rel_after_second
> -----------------+------------------
> 117 | 100
>
>
> We can see that after the first force, relation statistics contains 100 tuples,
> but the database aggregate does not (while the second force adds them).
>
> This is because it keeps the flushed database entry in the pending list. When the
> relation callback later adds counters to that already visited entry, it is not
> requeued because entry_ref->pending is non-NULL. Then, those counters wait
> until the next flush.
>
> That could also happen for custom stats that updates an already visited retained
> entry.
>
> One option could be to keep pending entry memory allocated, but track queue
> membership separately and requeue any entry that receives new counters after
> being processed.

I only looked at this finding so far. After spending some time
thinking about it, doing cross-kind accumulation inside
flush_pending_cb seems problematic by design.

```
bool
pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
...
....
/* The entry was successfully flushed, add the same to database stats */
dbentry = pgstat_prep_database_pending(dboid);
dbentry->tuples_returned += lstats->counts.tuples_returned;
dbentry->tuples_fetched += lstats->counts.tuples_fetched;
dbentry->tuples_inserted += lstats->counts.tuples_inserted;
dbentry->tuples_updated += lstats->counts.tuples_updated;
dbentry->tuples_deleted += lstats->counts.tuples_deleted;
dbentry->blocks_fetched += lstats->counts.blocks_fetched;
dbentry->blocks_hit += lstats->counts.blocks_hit;

return true;
```

Right now with only flushing at the end of transaction, we can get
away with it because flushed entries are deleted, so dependent
entries get re-created at the tail and flushed in the same pass.
But the mid-transaction case exposes the ordering dependency, since
entries are not deleted and not re-visited in the same pass.
As you call out, custom stats could hit this too.

I think the fix is to separate cross-kind accumulation from the
flush itself. Rather than having flush_pending_cb call
pgstat_prep_database_pending() while we're iterating the pending
list, what do you think of adding a post_flush_pending_cb that
runs after a successful flush? At a transaction boundary this
works naturally. The flushed relation entry gets deleted, the
callback re-creates the database entry, and is guaranteed to
be visited.

For mid-transaction flushing where entries stay on the list, a
dependent entry that was already visited won't be reached again
in the first pass because it's not a new entry. So, if we are
mid-transaction and still have pending data, we can take a second
pass to handle the stats accumulated during the post flush
callback.

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.

What do you think?

--
Sami