Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wcCfg-003AUS-1V for pgsql-bugs@arkaria.postgresql.org; Wed, 24 Jun 2026 01:44:56 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wcCee-00Ejfe-1D for pgsql-bugs@arkaria.postgresql.org; Wed, 24 Jun 2026 01:43:52 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wcCee-00EjfW-0F for pgsql-bugs@lists.postgresql.org; Wed, 24 Jun 2026 01:43:52 +0000 Received: from mail-pl1-x633.google.com ([2607:f8b0:4864:20::633]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1wcCeY-000000021TU-10Og for pgsql-bugs@lists.postgresql.org; Wed, 24 Jun 2026 01:43:51 +0000 Received: by mail-pl1-x633.google.com with SMTP id d9443c01a7336-2c6d31bfc8aso2646135ad.2 for ; Tue, 23 Jun 2026 18:43:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1782265424; cv=none; d=google.com; s=arc-20240605; b=IJHmkQDmtviDY+HOr2ta8zq+Gq5opBf33ePIfMK0szzB5VhyZdXji7Bj/8D7qgv+Kt NJOwGc0hCOpFHCPQG2HM6TvHGs739Et3ui8vZqSc0f+HniGyR5fBUzUOknqpdPQon8T/ ymw7Qa4g2KzXeS3s5+zH4owJ9nMg2TS9RpoAYJUSrgRLoZvOZa4+DHctVaAxYlaWGFEx CPMp5HxktoCrLeIjvWRbn7aQV3CBKNXxrFcT340fMTq/fgOg92/ONzRGR1IYdPl2faA6 dUS96GqNh3sITOa6smIG5yXpeQCN6VCdJV9Rior35vpFO7wfv/7DMrLHnRGOdqwIHNgt vzoA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:dkim-signature; bh=Eu3VijA/j6KAe0292aX7YcIqcZaoL0c/ulIQB2CA/nU=; fh=oozV6Gbrht2Z4ytUKsV363S48K/BBh/hJBpzMpnloZ0=; b=WZHqnpQFabMSlyBJCBn7DI0cGJH17vmXhDsdTKW3ub/BJzhbAh/9NYKtikVQUGcn6B dmhHfcQFNzvI8QmgOLnoTOV2KBZXGDQsZskbuLnFEh/m90xD/YFtaktFLgbm2iQZ6c1W Z4r5VAzaFPES7pnN4R9ujj+pktOrTDI9cMdzABcxkawo0RIAjLy72UOi5MTuBj4xWVRP sfRkNwUNvZs7knB163IenKZhhp6b+OcMVnIH0tbnq6nw1CWb83ieIUIZPaRZ+nAmepAR gjUrfD+TicwDFLh6V0NtQoSET43GBNuiAkzVP5rwR0AKILVBdgcHCcsXCA2QXcaYdnWh h6tg==; darn=lists.postgresql.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1782265424; x=1782870224; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Eu3VijA/j6KAe0292aX7YcIqcZaoL0c/ulIQB2CA/nU=; b=lM28nJRakJrnFbkbxdiHcbzaTQNrg/5XnvnlmA2PoAXBXNwxf6aM6o4/G37VdfY5EN wPjlMZF0NugNSWkQcldND+gBB2vUlid8dos1kPLPvX//axH13hqdfEL8wiqDc/EhE6hf P5o8FsUv9SYAl1AMbqBvRHJHXJiJRbxHhHOS4g5X1TiNWEWKXKie4XPi+il7/TPZEiwS tb3xDkOZT6APVbbgBbvqSEey4w6wcTjRI5KNrWSDpkU9d3laQPYvHO4g2jkjvd5wC01p 5oG+9sb/j3DrRcHF6GdAR0JtlTjEv/2ezyGsxFApWlIG3yqoJbFEJxtjKqrkMU0LWn5q 9ykg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1782265424; x=1782870224; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=Eu3VijA/j6KAe0292aX7YcIqcZaoL0c/ulIQB2CA/nU=; b=PSHPkFK8b6wLdT4PFaOJ7nS0jtXXWvYTPLC0dZMf0a9dLJqwgkwJRkTpDP2fl11U57 T759DieVDMyjfo9vVZ93o/4jUABirufsBwJJC1nK+DdqPs2G+nLole9fs8AuawenO9ke DiPonYXvXLEJ24dP74bHB35qOZnQt0dzMhbSq2g0gG8UT7nLL7qBy05biaA8LSBEbcyS A3Rr6XSC3FGxPtPsxqwCt7O6JDZbIhk6x4RO/lcTwUyzMU9saXBQq0g4Iv/Qb8BwgxI/ HnhbI1nWZzdgcCCQMJD9M25VIf5YoYOGzg/Npp3azQrsDWWspD7DRzZN50umbOQ2uvJ8 OY3w== X-Forwarded-Encrypted: i=1; AHgh+RrWsOt4FIptIcjm9H6WTwZ9AHyE/6K8QlgQLmYkm5cztYj72ZOmN/m7+WYhs3mzDDjuSkIYetoPxOjA@lists.postgresql.org X-Gm-Message-State: AOJu0YxJsZYS7ReFHnNXmh5xM7mxiWvKhDlOEpP7dt497t7fgWx9XQP+ fLULHcFxhrrOc86dRGTO1TGssHn7xnIUOFXpDxfmCtO5KjY6QOwyX/xJpAqzt1aqbntLY92aM8q cSQ/Z/fSZzVTjGkeQMoxXxWMwrJYUEio= X-Gm-Gg: AfdE7cmWdAYNAYasX1eO+EO3aWxZJCPOPG5yLh+BsHOWpbDhJmy60D2XHjWEzv92/IU X9j0yyLXPVgpjIN5i/OE5LWt2wA0SnnFxpWFE6VDTkzGLOYkw6qEM6Zx7MxFY8dgGium0vdnNX1 fEMpGg1d9IPGpeuYZ5WZ3SB+VP9IDUw/9sBdm1wa3oqpj/GfXHkQEV8nHHOpt89I3fBAcUrFCmK fPQ878qm6KyWIpwgwKmkSUPbVMS9ewe+oO5F5ivweMaGg7ove2cZQGREoV/Uwk4lIpVyXyRlFcX K7vSnZA5WQeFwdjDPeUsTlmGqJjNw6SE6zI6uOzzXOTy584EIFCUDG+U60O8H3GxiNCEJm8s8Dk 5r1Fnf80Vkw== X-Received: by 2002:a17:902:f54c:b0:2c6:bb31:4564 with SMTP id d9443c01a7336-2c7e15b2a18mr14529875ad.34.1782265423989; Tue, 23 Jun 2026 18:43:43 -0700 (PDT) MIME-Version: 1.0 References: <19484-a3cb82c8cde3c8fa@postgresql.org> In-Reply-To: From: Amit Langote Date: Wed, 24 Jun 2026 10:43:32 +0900 X-Gm-Features: AVVi8CcHwArR9ADbiyeIORKuGmYn9N_nts5s6UIjvDxPZWyzErNbwqrxSKZOx_s Message-ID: Subject: Re: BUG #19484: Segmentation fault triggered by FDW To: Etsuro Fujita Cc: Matheus Alcantara , Ayush Tiwari , Rafia Sabih , 798604270@qq.com, pgsql-bugs@lists.postgresql.org, Amit Langote Content-Type: multipart/alternative; boundary="0000000000007e10ab0654f60106" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --0000000000007e10ab0654f60106 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Jun 24, 2026 at 9:00 Amit Langote wrote: > On Tue, Jun 23, 2026 at 5:27=E2=80=AFPM Etsuro Fujita > wrote: > > On Mon, Jun 22, 2026 at 5:28=E2=80=AFPM Amit Langote > wrote: > > > On Fri, Jun 19, 2026 at 8:59=E2=80=AFPM Etsuro Fujita > wrote: > > > > > > +-- Runtime pruning of result relations must keep ModifyTable's > per-relation > > > > +-- FDW arrays (fdwPrivLists, fdwDirectModifyPlans) aligned with th= e > kept > > > > +-- resultRelations. Otherwise BeginForeignModify() reads the wron= g > > > > +-- fdw_private and segfaults. > > > > > > > > This comment isn't 100% correct, because this issue happens with > > > > DirectModify as well. I think we could expand the comment to menti= on > > > > that as well, but the comment is already too much/detailed IMO; I > > > > don't think we add such a comment for a test case, so how about > > > > simplifying the comment like this: "Test that direct modify and > > > > foreign modify work with runtime pruning of result relations (bug > > > > #19484)" > > > > > > I agree that the details shouldn't leak into test code and have been > > > trying to follow that principle. Also, bug numbers shouldn't be > > > mentioned because one can always use `git blame` to find them. > > > However, since different committers have different styles, I'm fine > > > with it; kept in the attached updated patch. > > > > Thanks! > > > > > > @@ -1446,6 +1446,12 @@ typedef struct ModifyTableState > > > > int mt_nrels; /* number of entries in > resultRelInfo[] */ > > > > ResultRelInfo *resultRelInfo; /* info about target relation(s= ) > */ > > > > > > > > + /* > > > > + * Re-indexed fdw private data lists, aligned with > resultRelInfo[] after > > > > + * pruning > > > > + */ > > > > + List *mt_fdwPrivLists; > > > > > > > > For v18, I think we should put this at the end of the > ModifyTableState > > > > struct, to avoid ABI breakage. > > > > > > Good point on ABI. Rather than placing it differently per branch, how > > > about putting mt_fdwPrivLists at the end of the struct alongside > > > mt_updateColnosLists/mt_mergeActionLists/mt_mergeJoinConditions, whic= h > > > are the same kind of unpruned-filtered lists? That keeps it ABI-safe > > > and lets master and 18 share an identical change. I'd fold it into > > > their existing comment too. > > > > That's a good idea! So +1 > > > > > Updated patch attached. > > > > LGTM. > > Pushed, thanks for checking. crake is now red on REL_18: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=3Dcrake&dt=3D2026-0= 6-24%2000%3A02%3A03 It reports sizeof(ModifyTableState) grew by 8 bytes. That=E2=80=99s from adding mt_fdwPrivLists at the end of the struct. Placin= g it last keeps existing field offsets stable, which is what extensions reading the node rely on, but it does grow the struct, which is what the checker flags. I believe we=E2=80=99ve added trailing struct members in back branch= es before for exactly this reason. Is a flagged sizeof increase acceptable here under that precedent, or do we now want to avoid any flagged ABI change on a back branch? If we=E2=80=99d rather avoid it, I have a no-field alternative for REL_18: show_modifytable_info() is the only reader of the re-indexed list, and it can recompute the mapping from node->resultRelations and node->fdwPrivLists, both plan-ordered and untouched by pruning. master would keep the field as committed, so the branches would differ slightly. I=E2=80=99m inclined toward the no-field version for REL_18 to keep the far= m green, unless someone would rather keep the same code in both branches and accept the size change. I=E2=80=99ll hold off briefly for any thoughts before doin= g that. - Amit > --0000000000007e10ab0654f60106 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Wed, Jun 24, 2026 at 9:00 Amit Langote <amitlangote09@gmail.com> wrote:
<= /div>
On Tue, Jun 23, 2026 at 5:27=E2=80=AFPM Etsuro F= ujita <etsu= ro.fujita@gmail.com> wrote:
> On Mon, Jun 22, 2026 at 5:28=E2=80=AFPM Amit Langote <amitlangote09@gmail.com= > wrote:
> > On Fri, Jun 19, 2026 at 8:59=E2=80=AFPM Etsuro Fujita <etsuro.fujita@gmail.c= om> wrote:
>
> > > +-- Runtime pruning of result relations must keep ModifyTabl= e's per-relation
> > > +-- FDW arrays (fdwPrivLists, fdwDirectModifyPlans) aligned = with the kept
> > > +-- resultRelations.=C2=A0 Otherwise BeginForeignModify() re= ads the wrong
> > > +-- fdw_private and segfaults.
> > >
> > > This comment isn't 100% correct, because this issue happ= ens with
> > > DirectModify as well.=C2=A0 I think we could expand the comm= ent to mention
> > > that as well, but the comment is already too much/detailed I= MO; I
> > > don't think we add such a comment for a test case, so ho= w about
> > > simplifying the comment like this: "Test that direct mo= dify and
> > > foreign modify work with runtime pruning of result relations= (bug
> > > #19484)"
> >
> > I agree that the details shouldn't leak into test code and ha= ve been
> > trying to follow that principle.=C2=A0 Also, bug numbers shouldn&= #39;t be
> > mentioned because one can always use `git blame` to find them. > > However, since different committers have different styles, I'= m fine
> > with it; kept in the attached updated patch.
>
> Thanks!
>
> > > @@ -1446,6 +1446,12 @@ typedef struct ModifyTableState
> > >=C2=A0 =C2=A0 =C2=A0int=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mt_n= rels;=C2=A0 =C2=A0 =C2=A0 =C2=A0/* number of entries in resultRelInfo[] */<= br> > > >=C2=A0 =C2=A0 =C2=A0ResultRelInfo *resultRelInfo;=C2=A0 =C2= =A0/* info about target relation(s) */
> > >
> > > +=C2=A0 =C2=A0/*
> > > +=C2=A0 =C2=A0 * Re-indexed fdw private data lists, aligned = with resultRelInfo[] after
> > > +=C2=A0 =C2=A0 * pruning
> > > +=C2=A0 =C2=A0 */
> > > +=C2=A0 =C2=A0List=C2=A0 =C2=A0 =C2=A0 =C2=A0*mt_fdwPrivList= s;
> > >
> > > For v18, I think we should put this at the end of the Modify= TableState
> > > struct, to avoid ABI breakage.
> >
> > Good point on ABI. Rather than placing it differently per branch,= how
> > about putting mt_fdwPrivLists at the end of the struct alongside<= br> > > mt_updateColnosLists/mt_mergeActionLists/mt_mergeJoinConditions, = which
> > are the same kind of unpruned-filtered lists? That keeps it ABI-s= afe
> > and lets master and 18 share an identical change. I'd fold it= into
> > their existing comment too.
>
> That's a good idea!=C2=A0 So +1
>
> > Updated patch attached.
>
> LGTM.

Pushed, thanks for checking.

crake is now red on REL_18: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=3Dcrake&dt= =3D2026-06-24%2000%3A02%3A03

It reports sizeof(ModifyTableState) = grew by 8 bytes.

That=E2=80=99s from adding mt_fdwPrivLists a= t the end of the struct. Placing it last keeps existing field offsets stabl= e, which is what extensions reading the node rely on, but it does grow the = struct, which is what the checker flags. I believe we=E2=80=99ve added trai= ling struct members in back branches before for exactly this reason. Is a f= lagged sizeof increase acceptable here under that precedent, or do we now w= ant to avoid any flagged ABI change on a back branch?

If we=E2=80=99d rather avoid it, I have a no= -field alternative for REL_18: show_modifytable_info() is the only reader o= f the re-indexed list, and it can recompute the mapping from node->resul= tRelations and node->fdwPrivLists, both plan-ordered and untouched by pr= uning. master would keep the field as committed, so the branches would diff= er slightly.

I=E2=80=99m inclined toward the no-field ver= sion for REL_18 to keep the farm green, unless someone would rather keep th= e same code in both branches and accept the size change. I=E2=80=99ll hold = off briefly for any thoughts before doing that.

- Amit
--0000000000007e10ab0654f60106--