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

Hi,

Thanks for the review!

> At minimum that should be documented but I think a cleaner fix would be to
> separate pending lifetime from queue membership, so that PGSTAT_FLUSH_DONE
> removes an entry from the work queue while retaining its storage.

After discussing offline, I don't think I like the idea of an
extension having to set a flag to force a re-scan. It's not
a very clear API, so your idea of re-queueing and for this
to happen transparently is important. Initially, I was hesitant
to manipulate the pending list, but after playing around with this,
I think it's doable and the best way to proceed. We can
have a flag on entry_ref called flushed_this_pass which is
set to true whenever we flush it, even partially, but not on a
lock conflict, where nothing was flushed. Then, whenever
pgstat_prep_pending_from_entry_ref() is called again and the
entry is seen as having been flushed, we move it to the tail
so the ongoing scan can find it again.

```
@@ -1388,8 +1396,18 @@
pgstat_prep_pending_from_entry_ref(PgStat_EntryRef *entry_ref)
}

entry_ref->pending =
MemoryContextAllocZero(pgStatPendingContext, entrysize);
+ entry_ref->flushed_this_pass = false;
dlist_push_tail(&pgStatPending, &entry_ref->pending_node);
}
+ else if (entry_ref->flushed_this_pass)
+ {
+ /*
+ * The entry is already pending and was already
visited in the current
+ * flush pass. Move it to the tail so the data just
accumulated into
+ * it is flushed again before the pass ends.
+ */
+ dlist_move_tail(&pgStatPending, &entry_ref->pending_node);
+ }
}
```

That is the crux of the fix. The flush loop clears the flag on the
current entry before invoking its callback, so a callback that
accumulates into its own entry does not re-queue itself, and the next
pointer is determined after the callback returns, so a re-queued entry
is always picked up by the ongoing scan.

With regards to the still relevant points you raised:

==

> I think that pgstat_report_analyze() should subtract only the unflushed
> delta, means:
>
> deadtuples -= rel->pgstat_info->counts.delta_dead_tuples - rel->pgstat_info->flushed.delta_dead_tuples;

done

==

> However, changed_tuples is cumulative and truncate does not reset the shared
> mod_since_analyze counter. Resetting flushed.changed_tuples to zero can
> therefore publish changes that were already published.
>
> I don't think flushed.changed_tuples should be reset here.

done.

===

> Both truncdropped values are then true. A subsequent truncate leaves
> counts.truncdropped true, so the condition does not become true again and
> the statistics reset is skipped.

done.

===

> I think that would need a bump catalog version, add a XXX in the commit
> message to not forget about it?
>
> also "descr => 'statistics: force stats to be flushed after the next
> commit'," should be updated?

done. The catalog version is bumped and the descr now reads
"statistics: force stats to be flushed, immediately if within a
transaction".

===

> Should we also add test to verify that the function double counting bug
> is solved?

done. stats.sql now has a test where a plpgsql function calls
pg_stat_force_next_flush() from within itself, then verifies that
pg_stat_user_functions and pg_stat_get_xact_function_calls() both
report the correct call counts mid-transaction and after commit, with
no calls lost or double counted.

Also, I am keeping the test_custom_stats changes out of the main patch for
now, but have them attached as a nocfbot as they may help in the patch review.
The incorporate your ideas for the same kind with different objects, etc.

Lastly, Since the mid-transaction behavior is now user visible, v5 also brings
back the documentation updates for last_seq_scan/last_idx_scan from an
earlier version, and adds pg_stat_force_next_flush() to the statistics
functions table in monitoring.sgml. It was previously undocumented as a
test only helper, but that no longer seems appropriate given it now has
a public facing behavior worth describing.

Horighuchi-san raised a point here [1] about throttling mid-transaction flushes,
but I am not sure if we should. These are manually executed, and I
think the caller
should be the one responsible for throttling, not the pgstat infrastructure.
WDYT?

Attached is v5.

[1] https://www.postgresql.org/message-id/20260601.135858.1116584574478485492.horikyota.ntt%40gmail.com

--
Sami Imseih
Amazon Web Services (AWS)