Re: PostgreSQL12 crash bug report

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2019-07-31 22:37:40 -0400, Tom Lane wrote:
>> Andres Freund <andres(at)anarazel(dot)de> writes:
>>> Previously we simply didn't need to know the type during EPQ setup,
>>> because we only stored a HeapTuple anyway. And we'd know the appropriate
>>> tupledesc at the places that access the tuple.

>> Right. So we gotta refactor that somehow.

> One way to avoid doing so would be to just not deal with slots in the
> case of ROW_MARK_COPY. The reason why EPQ was made to use to use slots
> instead of HeapTuples is that not doing so would require expensive
> conversions for AMs that don't use HeapTuple. But for COPY rowmarks
> that's not an issue, because the source already is a HeapTuple (in the
> form of a Datum).
> That's imo not a particularly pretty approach, but might be the most
> viable approach for v12. Attached is a quick WIP implementation, which
> indeed fixes the crash reported here, and passes all our
> tests. Obviously it'd need some polish if we went for that.

I agree that that's mighty ugly. But it does have the advantage of being
not very invasive, so at this late date maybe it's what we should do
for v12.

>>> One bigger change - but possibly one worth it - would be to basically
>>> move the work done in EvalPlanQualFetchRowMarks() to be done on-demand,
>>> at least for ROW_MARK_COPY.

>> Hm. Too tired to think that through right now.

> I briefly tried to implement that. The problem is that, as things are
> currently set up, we don't have access to the corresponding epqstate
> from within executor nodes. That prevents us from accessing
> EPQState->{origslot, arowMarks}, which we need to actually be able to
> fetch the rows to return.
> We could solve that by referencing the EPQState from its EState (but not
> from the "main" estate). I've wondered before whether that'd be better
> than having various es_epq* fields in EState. I don't think it'd make
> the modularity any worse, given that we're already referencing EPQstate
> fields from with EState. If we moved es_epqTupleSlot into EPQState and
> allocated it from within EvalPlanQualInit(), instead of
> EvalPlanQualBegin(), we'd address your complaint that we now call that
> earlier too.
> Doesn't sound that hard. Seems like a big change for v12 though.

I feel that this seems like a promising solution for the longer term.
I agree that keeping a pointer to the whole EPQState in EState is no
worse than having pointers to some of its fields there.

Perhaps the next step is to draft a patch for this and see just how
big it is compared to the minimal fix. I'd rather not have v12 be
different from later branches in this area if the change doesn't
seem entirely unreasonable for late beta.

regards, tom lane