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