Re: PostgreSQL12 crash bug report

On 2019-08-27 13:06:37 -0400, Tom Lane wrote:
> > Tom, everyone, what do you think of the attached approach?  It
> > implements what I described upthread.
> 
> Seems reasonable to store a pointer in the EPQState rather than pass
> that as a separate argument, but I think you need a bit more documentation
> work to keep the two EState pointers from being impossibly confusing.

Agreed.


> Also I dislike the field name "es_active_epq", as what that suggests
> to me is exactly backwards from the way you apparently are using it.
> Maybe "es_parent_epq" instead?  The comment for it is pretty opaque
> as well, not to mention that it misspells EState.

I think what I was trying to signal was that EPQ is currently active
from the POV of executor nodes. Ought to have been es_epq_active, for
that, I guess.  For me "if (estate->es_epq_active)" explains the meaning
of the check more than "if (estate->es_parent_epq)".


> I don't agree with the FIXME comment in execnodes.h.  The rowmarks
> state is used for things that are outside of EPQ, particularly the
> actual row locking ;-), so I wouldn't want to move that even if it
> didn't create notational issues.

Yea, I wasn't sure about that.


> I'm confused by the removal of the EvalPlanQualBegin call at
> nodeModifyTable.c:1373 ... why is that OK?

EvalPlanQual() calls EvalPlanQualBegin(), it was just needed to initiate
enough state to be able to get a slot.


> The original intent of EvalPlanQualInit was to do an absolutely
> minimal amount of work, since we don't know yet whether EPQ will
> ever be needed (and, generally, the way to bet is that it won't be).
> You've added some pallocs and list-to-array conversion, which
> while not terribly expensive still seem like they don't belong
> there.  Can't we move most of that to EvalPlanQualBegin?  I see that
> we want the substitute_slot array to exist so that EvalPlanQualSlot
> can work without EvalPlanQualBegin, but if we're postponing
> non-locking-rowmark work then we don't need the rest.

Yea, we can move the other fields without much trouble.


> > - Should the RTI indexed version of arowmarks be computed as an array
> >   directly in nodeModifyTable.c etc? Right now we build it first as a
> >   list, and then convert it as an array? We could also just use an rti
> >   indexed List, and store NULLs where there are no RTIs? But there's no
> >   good API for that.
> 
> Do that as a follow-on patch if at all.  I'm not sure it's worth changing.

Well, it'd at least partially address your concern above that we ought
to do less work during EvalPlanQualInit - if we had it in array form
we'd just need to set a variable, rather than do the transformation.

Greetings,

Andres Freund