Re: Assertion failure in HEAD and 13 after calling COMMIT in a stored proc

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.

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();