Em qua., 23 de jun. de 2021 às 21:45, Greg Nancarrow <gregn4422@gmail.com> escreveu:
On Wed, Jun 23, 2021 at 11:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > The code is correct as-is; the proposed change would result in taking > more snapshots than needed. Perhaps the comment needs revision, since > you both misread it. The comment is written in terms of "when can we > skip taking a snapshot", while the test in the code is written for > the inverse condition "when do we need a snapshot".
Yes, you're right. Even though I did realise that the comment was talking about the inverse, the condition for needing a snapshot still seemed too narrow, based on the comment, but checking the cases again, it is correct.
I still don't agree. But we leave the code like that, to see how it behaves.
Perhaps that code could have been written as the following, to better align with the comments:
skip_snapshot = (!expr->expr_simple_mutable || estate->readonly_func); if (!skip_snapshot) { ... }
...
if (!skip_snapshot) PopActiveSnapshot();
-1. That way it's readability is much worse, more complicated and IMHO it generates worse asm.
