Re: BUG #17804: Assertion failed in pg_stat after fetching from pg_stat_database and switching cache->snapshot

On Fri, Apr 28, 2023 at 04:07:52PM +0900, Kyotaro Horiguchi wrote:
> For the the record, I'm not saying that it is dangerous to clear
> snapshots directly in the callback. In fact, as I mentioned earlier, I
> believe there is no issue with that.  But, I believe it is simpler
> that the actual work is separate from the callback path since we don't
> need to worry about when the guc-callback will be called.

pgstat_clear_backend_activity_snapshot() and pgstat_assert_is_up() are
the two points of contention here.  The former could be called in a
non-backend context, which would be an incorrect use of this API.  I
am not completely sure that pgstat_assert_is_up() would be always
correct, as well, though I have not been able to see a problem.

Anyway, sorry for the late replay, it took me some time to study this
subsystem and understand what's going on here (like why we have fixed
snapshots but these don't list a timestamp, for example).  At the end,
I think that I'm OK with your suggestion to just force a snapshot
cleanup each time the GUC is changed, mainly because that's simple to
understand, and do the clear based on the timing where the next
snapshot build would happen.  I would be curious to hear arguments if
there would be a need for a more complex mechanism, but that seems
overkill to me as this is mainly here when switching out of the
"snapshot" mode.  A commit/abort would force a cleanup of the
snapshot, clearing the flag, as well.

I would add a small note in the docs about this behavior.  Another
thing is to add a few pg_stat_get_snapshot_timestamp() after building
a snapshot for each mode tested.

While on it, I have noticed that postgresql.conf.sample does not list
the values available for stats_fetch_consistency.  These had better be
added to the sample file, no?

At the end, I am finishing with the attached, without the sample file
part.  Thoughts?
--
Michael