On Fri, Aug 7, 2020 at 9:32 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > Andy Fan <zhihui.fan1...@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