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

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