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

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)