On Fri, Aug 7, 2020 at 9:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andy Fan <zhihui.fan1213@gmail.com> writes:
> > Attached is the v2 patch.
Thanks Andy and Tom for this.
> Forgot to mention that I'd envisioned adding this as a src/test/modules/
> module; contrib/ is for things that we intend to expose to users, which
> I think this isn't.
>
> I played around with this and got the isolation test I'd experimented
> with yesterday to work with it. If you revert 7a980dfc6 then the
> attached patch will expose that bug. Interestingly, I had to add an
> explicit AcceptInvalidationMessages() call to reproduce the bug; so
> apparently we do none of those between planning and execution in the
> ordinary query code path. Arguably, that means we're testing a scenario
> somewhat different from what can happen in live databases, but I think
> it's OK. Amit's recipe for reproducing the bug works because there are
> other relation lock acquisitions (and hence AcceptInvalidationMessages
> calls) later in planning than where he asked us to wait. So this
> effectively tests a scenario where a very late A.I.M. call within the
> planner detects an inval event for some already-planned relation, and
> that seems like a valid-enough scenario.
Agreed.
Curiously, Justin mentioned upthread that the crash occurred during
BIND of a prepared query, so it better had been that a custom plan was
being executed, because a generic one based on fewer partitions would
be thrown away due to A.I.M. invoked during AcquireExecutorLocks().
> Anyway, attached find a reviewed version of your patch plus a test
> scenario contributed by me (I was too lazy to split it into two
> patches). Barring objections, I'll push this tomorrow or so.
>
> (BTW, I checked and found that this test does *not* expose the problems
> with Amit's original patch. Not sure if it's worth trying to finagle
> it so it does.)
I tried to figure out a scenario where my patch would fail but
couldn't come up with one either, but it's no proof that it isn't
wrong. For example, I can see that pinfo->relid_map[pinfo->nparts]
can be accessed with my patch which is not correct.
--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com
