From: | Amit Langote <amitlangote09(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: FailedAssertion("pd_idx == pinfo->nparts", File: "execPartition.c", Line: 1689) |
Date: | 2020-08-07 03:16:11 |
Message-ID: | CA+HiwqGr=vPwPfwKcc+biEc5LxhLE4fWYnBGmzv-vUNq+_fp+Q@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Aug 7, 2020 at 9:32 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andy Fan <zhihui(dot)fan1213(at)gmail(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2020-08-07 03:31:28 | Re: FailedAssertion("pd_idx == pinfo->nparts", File: "execPartition.c", Line: 1689) |
Previous Message | Noah Misch | 2020-08-07 03:00:20 | Re: public schema default ACL |