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

On Tue, Jun 22, 2021 at 10:56 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
>
> On Mon, Jun 21, 2021 at 04:19:27PM -0700, Jim Nasby wrote:
> > The following generates an assertion failure. Quick testing with start and
> > stop as well as the core dump shows it’s failing on the execution of
> > `schema_name := schema_name(i)` immediately after COMMIT, because there’s no
> > active snapshot. On a build without asserts I get a failure in
> > GetActiveSnapshot() (second stack trace). This works fine on 12_STABLE, but
> > fails on 13_STABLE and HEAD.
>
> For me it's a typo.
> need_snapshot = (expr->expr_simple_mutable || !estate->readonly_func);
>
...
>
> The comments in the function are clear:
> If expression is mutable OR is a non-read-only function, so need a snapshot.
>

I have to agree with you.
Looks like the "&&" should really be an "||". The explanation in the
code comment is pretty clear on this, as you say.

I was able to reproduce the problem using your example. It produced a
coredump, pointing to the failed "Assert(ActiveSnapshotSet());" in
pg_plan_query().
I also verified that your patch seemed to fix the problem.

However, I found that this issue is masked by the following recent commit:

commit d102aafb6259a6a412803d4b1d8c4f00aa17f67e
Author: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Date: Tue Jun 22 17:48:39 2021 -0400
Restore the portal-level snapshot for simple expressions, too.

With this commit in place, there is an active snapshot in place
anyway, so for your example, that Assert no longer fires as it did
before.
However, I still think that your fix is valid and needed.

Regards,
Greg Nancarrow
Fujitsu Australia