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

On Mon, Jan 18, 2021 at 4:13 PM David Geier <david(at)swarm64(dot)com> wrote:
>
> Hi hackers,
>
> While working with cursors that reference plans with CustomScanStates
> nodes, I encountered a segfault which originates from
> search_plan_tree(). The query plan is the result of a simple SELECT
> statement into which I inject a Custom Scan node at the root to do some
> post-processing before returning rows. This plan is referenced by a
> second plan with a Tid Scan which originates from a query of the form
> DELETE FROM foo WHERE CURRENT OF my_cursor;
>
> 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.
>
> I attached a patch. Any thoughts?

I don't have any comments about your patch as such, but ForeignScan is
similar to CustomScan. ForeignScan also can leave ss_currentRelation
NULL if it represents join between two foreign tables. So either
ForeignScan has the same problem as CustomScan (it's just above
CustomScan case in search_plan_tree()) or it's handling it in some
other way. In the first case we may want to fix that too in the same
manner (not necessarily in the same patch) and the in the later case
CustomScan can handle it the same way.

Said that, I didn't notice any field in ForeignScan which is parallel
to custom_ps, so what you are proposing is still needed.

--
Best Wishes,
Ashutosh Bapat