On Wed, Jul 10, 2019 at 3:38 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > On Tue, Jul 9, 2019 at 1:13 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> >> I think it's highly likely that bug #15720 was a case of this bug and
> >> would be fixed by this patch.
>
> Agreed. I think your version of the fix is good, and you should
> mention #15720 too in the commit message.
Thanks. Pushed.
> >> Alex's repro doesn't work on 11 though,
> >> because EPQ is not entered at all. Which raises the question: why do
> >> we need to enter EPQ after commit ad0bda5d on 12/master, for a row
> >> that hasn't been updated by anyone else?
>
> > Explanation: since ad0bda5d24ea, ExecLockRows() always calls
> > EvalPlanQualBegin() which initialises the plan state, and in this case
> > ExecInitNamedTuplestoreScan() errors out due to the bug. Before, you
> > needed the right concurrency scenario (epq_needed) before we did that,
> > as the reporter of bug #15720 discovered.
>
> I'm quite desperately unhappy about this observation, because
> EvalPlanQualBegin is a *large* amount of overhead that is usually
> unnecessary, and is now going to be paid for *every locked row*
> whether there's any conflict on it or not. I do not find that
> acceptable. Why is it necessary to do this before finding that
> there's an update conflict?
I haven't seriously looked into it and haven't succeeded in finding
the discussion of why this is absolutely necessary in the commit's
thread, but you'd think it should be possible to defer slot creation a
bit, or if not, do something cheaper than EPQBegin() that just
initialises the slots. Andres, Haribabu, Ashutosh?
--
Thomas Munro
https://enterprisedb.com