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
