Re: search_plan_tree(): handling of non-leaf CustomScanState nodes causes segfault

Hi,
It seems sstate->ss_currentRelation being null can only
occur for T_ForeignScanState and T_CustomScanState.

What about the following change ?

Cheers

diff --git a/src/backend/executor/execCurrent.c
b/src/backend/executor/execCurrent.c
index 0852bb9cec..56e31951d1 100644
--- a/src/backend/executor/execCurrent.c
+++ b/src/backend/executor/execCurrent.c
@@ -325,12 +325,21 @@ search_plan_tree(PlanState *node, Oid table_oid,
case T_IndexOnlyScanState:
case T_BitmapHeapScanState:
case T_TidScanState:
+ {
+ ScanState *sstate = (ScanState *) node;
+
+ if (RelationGetRelid(sstate->ss_currentRelation) ==
table_oid)
+ result = sstate;
+ break;
+ }
+
case T_ForeignScanState:
case T_CustomScanState:
{
ScanState *sstate = (ScanState *) node;

- if (RelationGetRelid(sstate->ss_currentRelation) ==
table_oid)
+ if (sstate->ss_currentRelation &&
+ RelationGetRelid(sstate->ss_currentRelation) ==
table_oid)
result = sstate;
break;
}

On Mon, Jan 18, 2021 at 10:46 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> David Geier <david(at)swarm64(dot)com> writes:
> > search_plan_tree() assumes that
> > CustomScanState::ScanState::ss_currentRelation is never NULL. In my
> > understanding that only holds for CustomScanState nodes which are at the
> > bottom of the plan and actually read from a relation. CustomScanState
> > nodes which are not at the bottom don't have ss_currentRelation set. I
> > believe for such nodes, instead search_plan_tree() should recurse into
> > CustomScanState::custom_ps.
>
> Hm. I agree that we shouldn't simply assume that ss_currentRelation
> isn't null. However, we cannot make search_plan_tree() descend
> through non-leaf CustomScan nodes, because we don't know what processing
> is involved there. We need to find a scan that is guaranteed to return
> rows that are one-to-one with the cursor output. This is why the function
> doesn't descend through join or aggregation nodes, and I see no argument
> by which we should assume we know more about what a customscan node will
> do than we know about those.
>
> So I'm inclined to think a suitable fix is just
>
> - if (RelationGetRelid(sstate->ss_currentRelation) ==
> table_oid)
> + if (sstate->ss_currentRelation &&
> + RelationGetRelid(sstate->ss_currentRelation) ==
> table_oid)
> result = sstate;
>
> regards, tom lane
>
>
>