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
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?
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.
--
Michael