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

At Fri, 28 Apr 2023 13:37:23 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in
> On Fri, Apr 28, 2023 at 07:00:01AM +0300, Alexander Lakhin wrote:
> > 28.04.2023 04:41, Kyotaro Horiguchi wrote:
> >> Nonetheless, I'm a bit concerned about making a direct call to
> >> pgstat_clear_snapshot() from the assign callback, it might be fine for
> >> now, but I worry that it could an issue later on.
> >>
> >> So, how about just settin a trigger that causes a snapshot clearing
> >> prior to the next use, like the attached?
> >
> > I'm agree with the change ― it's still rather simple and fixes the issue.
> > Maybe you would also find appropriate to update the comment for
> > pgstat_get_stat_snapshot_timestamp().
> > Perhaps add something like?:
> > Note that the snapshot may be cleared here, if requested.
>
> FWIW, I was looking at this patch and this proposal of relying on a
> static flag to conditionally clear a snapshot looks rather brittle by
> design to me because this relies on quite a few assumptions that the
> snapshot will always be cleared when necessary, as proved by the two
> code paths of pgstat.c patched where each gain a check on

pgstat_get_stat_snapshot_timestmp() necessarily requires that. Just
for seeming consistency. The only significant part is
gstat_prep_snapshot, which is always called when the caller wants a
snapshot.

> clear_existing_snapshot. The AtEOXacts for pgstat when doing an
> abort/commit/prepare would guarantee that the flag is cleared, still
> that's not really exciting. What is the problem in forcing a snapshot
> cleanup each time stats_fetch_consistency is switched to a new value?

Just I wanted not to do that much in the guc callback including memory
context operations. If it is compeltly safe, I don't object just
clearing snapshots in the callback.

> Somebody using SET LOCAL on that means, at least it sounds to me as
> such, that they don't really care about the current snapshot contents
> so they'd better be wiped out. And the cleanup timing does not really
> seem to matter much.
>
> >> As for the test, I can't come up with a better one, but I think the
> >> comment should explain its intention more clearly.
> >
> > I'd hesitated to mention a crash in the comment because I've seen
> > assertion failures only (and no crash with a non-assert build), so I
> > thought that the test should illustrate the new behavior in the first place.
> > But I'm not opposed to your comment change, maybe it's more prominent indeed.
>
> If the issue is fixed, there would be no crash.

If crash is not appropriate there, I am fine with "changing the
variable clears snapshot".

Regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center