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@paquier.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