Re: BUG #19484: Segmentation fault triggered by FDW

Fujita-san,

On Thu, Jun 25, 2026 at 2:02 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Wed, Jun 24, 2026 at 8:20 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Wed, Jun 24, 2026 at 6:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > > +           /*
> > > +            * node->fdwPrivLists is indexed by the original, pre-pruning
> > > +            * result relation order and is parallel to node->resultRelations.
> > > +            * Initial pruning may have dropped earlier relations, so the kept
> > > +            * index j need not match the original position; find this
> > > +            * relation's entry by its range table index instead.
> > > +            */
> > > +           forboth(lc1, node->resultRelations, lc2, node->fdwPrivLists)
> > > +           {
> > > +               if (lfirst_int(lc1) == (int) rti)
> > > +               {
> > > +                   fdw_private = (List *) lfirst(lc2);
> > > +                   break;
> > > +               }
> > > +           }
> > >
> > > I think it's good to skip this for efficiency, when there are no
> > > pruned result relations.
> >
> > Thanks for the review. v2 attached implements your suggestion: it
> > indexes node->fdwPrivLists directly with j when no result relations
> > were pruned, and only falls back to matching by range table index when
> > pruning appears to have dropped some.
>
> Thank you for doing that work!
>
> This might be nitpicking, but:
>
> +           if (list_length(node->resultRelations) == mtstate->mt_nrels)
> +               fdw_private = (List *) list_nth(node->fdwPrivLists, j);
> +           else
> +           {
> +               Index       rti = resultRelInfo->ri_RangeTableIndex;
> +               ListCell   *lc1;
> +               ListCell   *lc2;
> +
> +               fdw_private = NIL;
> +               forboth(lc1, node->resultRelations, lc2, node->fdwPrivLists)
> +               {
> +                   if (lfirst_int(lc1) == (int) rti)
> +                   {
> +                       fdw_private = (List *) lfirst(lc2);
> +                       break;
> +                   }
> +               }
> +           }
>
> I'd put the if-test outside of the outer loop to save cycles.

Right, it's loop-invariant. v3 attached computes a boolean
(nopruning), like labeltargets, once before the loop and uses it
inside.

> Other than that v2 looks good to me.
>
> (The forboth loop actually causes an n-squared calculation, but it's
> done only when pruning occurs, in which case the number of remaining
> result relations would be reduced, so that wouldn't be a problem.)

Right, though strictly the inner forboth scans node->resultRelations,
which pruning leaves at its original length, so it's the original
relation count that bounds the scan rather than the reduced one.
Either way it's EXPLAIN-only with small counts, so it's not a concern.

Thanks again for the review.

-- 
Thanks, Amit Langote