Re: search_plan_tree(): handling of non-leaf CustomScanState nodes causes segfault - Mailing list pgsql-hackers

From Zhihong Yu
Subject Re: search_plan_tree(): handling of non-leaf CustomScanState nodes causes segfault
Date
Msg-id CALNJ-vRRwwqKuSvWJNdHG7Six0ZBt=LDVdrFfSitStuKHbjUkg@mail.gmail.com
Whole thread Raw
In response to Re: search_plan_tree(): handling of non-leaf CustomScanState nodes causes segfault  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: search_plan_tree(): handling of non-leaf CustomScanState nodes causes segfault  (Tom Lane <tgl@sss.pgh.pa.us>)
List pgsql-hackers
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@sss.pgh.pa.us> wrote:
David Geier <david@swarm64.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


pgsql-hackers by date:

Previous
From: Andres Freund
Date:
Subject: Re: Key management with tests
Next
From: Robert Haas
Date:
Subject: Re: CheckpointLock needed in CreateCheckPoint()?