Hi, Thanks for the review!
> That causes issues for function stats. pgstat_function_flush_cb() returns
> PGSTAT_FLUSH_DONE, then pgstat_delete_pending_entry() is called, freeing
> storage
> still referenced by PgStat_FunctionCallUsage.fs. When the function returns,
> pgstat_end_function_usage() writes through that stale pointer.
Good catch.
> I think that entry_ref->pending should not be freed by an in-transaction
> flush.
Agreed. In v3 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;
Since the entry stays alive mid-transaction, callbacks that return
PGSTAT_FLUSH_DONE must zero their local counters after flushing to
avoid double-counting on the next flush. This was always the right
thing to do, but the immediate freeing of the entry did not make
this a strict requirement, and only pgstat_database_flush_cb already
did this. Now it's required for mid-transaction flushes, so I added memset()
after flush in pgstat_function_flush_cb, pgstat_relation_flush_cb,
pgstat_subscription_flush_cb, and the test_custom_stats example.
> should we do the same for flush_static_cb() or exclude static callbacks from
> mid-transaction flushes? (for the same reason that xact_boundary has been
> added
> to flush_pending_cb())
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.
> Previously, flush_pending_cb returned a bool with that meaning: false for lock
> conflict and true for done.
> I think that would be better to change the enum ordering to preserve the
> previous
> meaning. The concern is silent misbehavior in custom callbacks.
Agreed. In v3 I reordered the enum so PGSTAT_FLUSH_LOCK_CONFLICT is
first, with an explicit = 0 and a comment explaining why.
I also added a regression test that exercises the scenario you
described, a function calling pg_stat_force_next_flush() from within
itself.
One caveat is zeroing total_time can cause double-counting if a
recursive function calls pg_stat_force_next_flush(). This isn't
ideal, but I don't think we should handle more of these edge
cases for pg_stat_force_next_flush(). The only way I can think
of dealing with this is to flush total_time at xact_boundary, but
that will need to be done for all cases, which is not ideal, IMO.
v3 attached.
--
Sami Imseih
Amazon Web Services (AWS)
v3-0001-pgstat-Allow-pg_stat_force_next_flush-to-work-in-.patch
Description: Binary data
