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)
nocfbot.test_custom_stats.patch
Description: Binary data
v5-0001-pgstat-Allow-pg_stat_force_next_flush-to-work-in-.patch
Description: Binary data
