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

Em qua., 23 de jun. de 2021 às 03:04, Greg Nancarrow <gregn4422(at)gmail(dot)com>
escreveu:

> 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().
>
Yes before d102aafb6, Jim Nasby example fires a coredump.

I also verified that your patch seemed to fix the problem.
>
Both fix the Jim example.

> 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.
>
I agreed.
Before 84f5c29, only the not-read-only function NOT push a new snapshot.
Now only mutable expression AND not-read-only function, pushes a new
snapshot.
Under which conditions did Jim's example not fit?

With && is very restricted.
We have:
1. Mutable expression AND not-read-only function -> push a new snapshot
2. Mutable expression AND read-only-function -> not push a new snapshot
3. Not mutable expression AND not-read-only function -> not push a new
snapshot
4. Not mutable expression AND read-only function -> not push a new snapshot

We agree that 2 and 3 should push a new snapshot.

If the user function is declared as not-read-only, even though read-only,
it's a failure to be fixed either by the user, or by the parser, not here.

regards,
Ranier Vilela