Re: postgres_fdw: wrong results with self join + enable_nestloop off

Hi,

On Sun, Aug 20, 2023 at 4:34 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2023-08-19 20:09:25 +0900, Etsuro Fujita wrote:
> > Maybe my explanation was not enough, so let me explain:
> >
> > * I think you could use the set_join_pathlist_hook hook as you like at
> > your own responsibility, but typical use cases of the hook that are
> > designed to support in the core system would be just add custom paths
> > for replacing joins with scans, as described in custom-scan.sgml (this
> > note is about set_rel_pathlist_hook, but it should also apply to
> > set_join_pathlist_hook):
> >
> >     Although this hook function can be used to examine, modify, or remove
> >     paths generated by the core system, a custom scan provider will typically
> >     confine itself to generating <structname>CustomPath</structname>
> > objects and adding
> >     them to <literal>rel</literal> using <function>add_path</function>.
>
> That supports citus' use more than not: "this hook function can be used to
> examine ... paths generated by the core system".
>
>
> > * The problem we had with the set_join_pathlist_hook hook is that in
> > such a typical use case, previously, if the replaced joins had any
> > pseudoconstant clauses, the planner would produce incorrect query
> > plans, due to the lack of support for handling such quals in
> > createplan.c.  We could fix the extensions side, as you proposed, but
> > the cause of the issue is 100% the planner's deficiency, so it would
> > be unreasonable to force the authors to do so, which would also go
> > against our policy of ABI compatibility.  So I fixed the core side, as
> > in the FDW case, so that extensions created for such a typical use
> > case, which I guess are the majority of the hook extensions, need not
> > be modified/recompiled.  I think it is unfortunate that that breaks
> > the use case of the Citus extension, though.
>
> I'm not neutral - I don't work on citus, but work in the same Unit as
> Onder. With that said: I don't think that's really a justification for
> breaking a pre-existing, not absurd, use case in a minor release.
>
> Except that this was only noticed after it was released in a set of minor
> versions, I would say that 6f80a8d9c should just straight up be reverted.
> Skimming the thread there wasn't really any analysis done about breaking
> extensions etc - and that ought to be done before a substantial semantics
> change in a somewhat commonly used hook.  I'm inclined to think that that
> might still be the right path.
>
>
> > BTW: commit 9e9931d2b removed the restriction on the call to the hook
> > extensions, so you might want to back-patch it.
>
> Citus is an extension, not a fork, there's not really a way to just backpatch
> a random commit.
>
>
> > Though, I think it would be better if the hook was well implemented from the
> > beginning.
>
> Sure, but that's neither here nor there.
>
> Greetings,
>
> Andres Freund