Re: PostgreSQL12 crash bug report

Hi,

On 2019-08-25 22:36:52 -0700, Andres Freund wrote:
> On 2019-08-24 15:54:01 -0700, Andres Freund wrote:
> > Working on that.
>
> Think it's going to look not too invasive. Needs a good bit more
> cleanup, but I'm getting there.
>
>
> One thing I'm struggling with is understanding and testing nested EPQ
> states:
>
> /*
> * Each EState must have its own es_epqScanDone state, but if we have
> * nested EPQ checks they should share es_epqTupleSlot arrays. This
> * allows sub-rechecks to inherit the values being examined by an outer
> * recheck.
> */
>
> At the moment I don't understand what the point here is. When/why do we
> want to inherit values from the parent epqstate? I know how to construct
> queries with nested EPQ, but I don't see why we'd ever want to inherit
> the values from the outer check? Nor how we're doing so -
> EvalPlanQualFetchRowMarks() unconditionally clears out the old tuple,
> and nodeLockRows.c unconditionally fetches the source tuples too?

As far as I can tell it's not worth preserving this bit - it doesn't
seem to be working in a way one might infer from the comment, and the
current behaviour is pretty weird: In a nested EPQState, we'll overwrite
the tuples accessed by an outer layer of EPQ. I'm not sure, but I could
even see that causing crash type problems, e.g. if a columns from the
slot fetched by the outer EPQ is passed to the inner EPQ, which then
stores a new tuple in that slot, potentially leaving the argument be a
dangling pointer into that tuple.

Second, and somewhat related, question:

Why are we passing the surrounding EState to both EvalPlanQualInit() and
to EvalPlanQual()? Seems to not make much sense anymore? As I needed to
have accesses to the parent EState from the EPQState, to be able to
allocate slots for EvalPlanQualSlot() before EvalPlanQualBegin() has
been called, that seems even less necessary.

I've verified that the attached fixes the crash reported upthread. All
tests pass. I think it needs a bit of cleanup, and a lot more test
coverage.

On the latter point, it seems that we are missing coverage around:
1) most of ROW_MARK_COPY
2) no nested EPQ
3) index only scan as an EPQ source
4) FDWs

I'll not be able to add complete coverage, but clearly we need more than
now.

Tom, everyone, what do you think of the attached approach? It
implements what I described upthread.

The problem with fixing this bug is that we currently don't know the row
types for ExecAuxRowMarks at the site that is calling EvalPlanQual, and
that that is not easy to fix. But we don't really need the type that
early as there is no real need to fetch row marks that early. The
difficulty moving it to ExecScanFetch is that at that stage we currently
don't access to the information about row marks from the EPQState.

To fix that, this patch moves most of the EPQ related information out of
the ESTate into the EPQState, and sets es_active_epq to the EPQState for
ESTates that are created by EvalPlanQualStart(). That then allows us to
have ExecScanFetch() fetch the rowmark on demand with the new
EvalPlanQualFetchRowMark (which basically is just the old
EvalPlanQualFetchRowMarks without a loop).

For EvalPlanQualFetchRowMark to work efficiently, I added an rti - 1
indexed EPQState->substitute_rowmark.

This also fixes Tom's complain elswhere that nodeLockRows.c does
EvalPlanQualBegin() earlier, by allowing EvalPlanQualSlot() to be called
before that. That seems considerably nicer to me.

Questions:
- Is there any potential downside to not fetching non-locking rowmarks
eagerly? It's observable for FDWs, but it shouldn't matter? As far as
I can tell the new behaviour should always be at least as efficient as
before?
- Other names for EPQState->substitute_*?
- 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.
- We currently - and haven't in recent history - free epq test tuples
until EPQ is needed the next time. Perhaps we should? But that'd
probably not be a candidate for backpatching.

Todo:
- tests
- remove isolationtester rescheduling

Greetings,

Andres Freund