On Tue, Jun 22, 2021 at 10:56 PM Ranier Vilela <ranier.vf@gmail.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@sss.pgh.pa.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
