Обсуждение: postgres_fdw: wrong results with self join + enable_nestloop off

Hi Nishant,

On Fri, Apr 14, 2023 at 8:39 PM Nishant Sharma
<nishant.sharma@enterprisedb.com> wrote:
> I debugged this issue and was able to find a fix for the same. Kindly please refer to the attached fix. With the fix
Iam able to resolve the issue. 

Thanks for the report and patch!

> What is the technical issue?
> The problem here is the use of extract_actual_clauses. Because of which the plan creation misses adding the second
conditionof AND i.e "now() < '23-Feb-2020'::timestamp" in the plan. Because it is not considered a pseudo constant and
extract_actual_clauseis passed with false as the second parameter and it gets skipped from the list. As a result that
conditionis never taken into consideration as either one-time filter (before or after) or part of SQL remote execution 
>
> Why do I think the fix is correct?
> The fix is simple, where we have created a new function similar to extract_actual_clause which just extracts all the
conditionsfrom the list with no checks and returns the list to the caller. As a result all conditions would be taken
intoconsideration in the query plan. 

I think that the root cause for this issue would be in the
create_scan_plan handling of pseudoconstant quals when creating a
foreign-join (or custom-join) plan.  Anyway, I will look at your patch
closely, first.

Best regards,
Etsuro Fujita

On Mon, Apr 24, 2023 at 3:31 PM Nishant Sharma
<nishant.sharma@enterprisedb.com> wrote:
> Any updates? -- did you get a chance to look into this?

Sorry, I have not looked into this yet, because I have been busy with
some other work recently.  I  plan to do so early next week.

Best regards,
Etsuro Fujita

Hi,

On Fri, Jun 2, 2023 at 9:31 PM Nishant Sharma
<nishant.sharma@enterprisedb.com> wrote:
> I also agree that Richard's patch is better. As it fixes the issue at the backend and does not treat pseudoconstant
aslocal condition. 
>
> I have tested Richard's patch and observe that it is resolving the problem. Patch looks good to me as well.

If the patch is intended for HEAD only, I also think it goes in the
right direction.  But if it is intended for back branches as well, I
do not think so, because it would cause ABI breakage due to changes
made to the ForeignPath struct and the create_foreign_join_path() API.
(For the former, I think we could avoid doing so by adding the new
member at the end of the struct, not in the middle, though.)

To avoid this issue, I am wondering if we should modify
add_paths_to_joinrel() in back branches so that it just disallows the
FDW to consider pushing down joins when the restrictlist has
pseudoconstant clauses.  Attached is a patch for that.

My apologies for not reviewing your patch and the long long delay.

Best regards,
Etsuro Fujita

Hi Richard,

On Tue, Jun 6, 2023 at 12:20 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Mon, Jun 5, 2023 at 9:19 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> To avoid this issue, I am wondering if we should modify
>> add_paths_to_joinrel() in back branches so that it just disallows the
>> FDW to consider pushing down joins when the restrictlist has
>> pseudoconstant clauses.  Attached is a patch for that.

> I think we can do that in back branches.  But I'm a little concerned
> that we'd miss a better plan if FDW cannot push down joins in such
> cases.  I may be worrying over nothing though if it's not common that
> the restrictlist has pseudoconstant clauses.

Yeah, it is unfortunate that we would not get better plans.  Given
that it took quite a long time to find this issue, I suppose that
users seldom do foreign joins with pseudoconstant clauses, though.

Anyway thanks for working on this, Richard!

Best regards,
Etsuro Fujita

Hi,

On Wed, Jun 7, 2023 at 7:28 PM Nishant Sharma
<nishant.sharma@enterprisedb.com> wrote:
> Etsuro's patch is also showing the correct output for "set enable_nestloop=off". Looks good to me for back branches
dueto backport issues. 
>
> But below are a few observations for the same:-
> 1) I looked into the query plan for both "set enable_nestloop" on & off case and observe that they are the same. That
is,what we see with "set enable_nestloop=on". 
> 2) In back branches for "set enable_nestloop" on & off value, at least this type of query execution won't make any
difference.No comparison of plans to be selected based on total cost of two plans old (Nested Loop with Foreign Scans)
&new (Only Foreign Scan) will be done, because we are avoiding the call to "postgresGetForeignJoinPaths()" up front
whenwe have pseudo constants. 

Thanks for looking!

Best regards,
Etsuro Fujita

On Mon, Jun 5, 2023 at 10:19 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> To avoid this issue, I am wondering if we should modify
> add_paths_to_joinrel() in back branches so that it just disallows the
> FDW to consider pushing down joins when the restrictlist has
> pseudoconstant clauses.  Attached is a patch for that.

I think that custom scans have the same issue, so I modified the patch
further so that it also disallows custom-scan providers to consider
join pushdown in add_paths_to_joinrel() if necessary.  Attached is a
new version of the patch.

Best regards,
Etsuro Fujita

Hi Richard,

On Sun, Jun 25, 2023 at 3:05 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Wed, Jun 14, 2023 at 2:49 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> On Mon, Jun 5, 2023 at 10:19 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> > To avoid this issue, I am wondering if we should modify
>> > add_paths_to_joinrel() in back branches so that it just disallows the
>> > FDW to consider pushing down joins when the restrictlist has
>> > pseudoconstant clauses.  Attached is a patch for that.
>>
>> I think that custom scans have the same issue, so I modified the patch
>> further so that it also disallows custom-scan providers to consider
>> join pushdown in add_paths_to_joinrel() if necessary.  Attached is a

> Good point.  The v2 patch looks good to me for back branches.

Cool!  Thanks for looking!

> I'm wondering what the plan is for HEAD.  Should we also disallow
> foreign/custom join pushdown in the case that there is any
> pseudoconstant restriction clause, or instead still allow join pushdown
> in that case?  If it is the latter, I think we can do something like my
> patch upthread does.  But that patch needs to be revised to consider
> custom scans, maybe by storing the restriction clauses also in
> CustomPath?

I think we should choose the latter, so I modified your patch as
mentioned, after re-creating it on top of my patch.  Attached is a new
version (0002-Allow-join-pushdown-even-if-pseudoconstant-quals-v2.patch).
I am attaching my patch as well
(0001-Disable-join-pushdown-if-pseudoconstant-quals-v2.patch).

Other changes made to your patch:

* I renamed the new member of the ForeignPath struct to
fdw_restrictinfo.  (And I named that of the CustomPath struct
custom_restrictinfo.)

* In your patch, only for create_foreign_join_path(), the API was
modified so that the caller provides the new member of ForeignPath,
but I modified that for
create_foreignscan_path()/create_foreign_upper_path() as well, for
consistency.

* In this bit I changed the last argument to NIL, which would be
nitpicking, though.

@@ -1038,7 +1038,7 @@ postgresGetForeignPaths(PlannerInfo *root,
  add_path(baserel, (Path *) path);

  /* Add paths with pathkeys */
- add_paths_with_pathkeys_for_rel(root, baserel, NULL);
+ add_paths_with_pathkeys_for_rel(root, baserel, NULL, NULL);

* I dropped this test case, because it would not be stable if the
system clock was too slow.

+-- bug due to sloppy handling of pseudoconstant clauses for foreign joins
+EXPLAIN (VERBOSE, COSTS OFF)
+  SELECT * FROM ft2 a, ft2 b
+  WHERE b.c1 = a.c1 AND now() < '25-April-2023'::timestamp;
+SELECT * FROM ft2 a, ft2 b
+WHERE b.c1 = a.c1 AND now() < '25-April-2023'::timestamp;

That is it.

Sorry for the long long delay.

Best regards,
Etsuro Fujita

Hi Richard,

On Mon, Jul 24, 2023 at 11:45 AM Richard Guo <guofenglinux@gmail.com> wrote:
> On Fri, Jul 21, 2023 at 8:51 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> * In this bit I changed the last argument to NIL, which would be
>> nitpicking, though.
>>
>> @@ -1038,7 +1038,7 @@ postgresGetForeignPaths(PlannerInfo *root,
>>   add_path(baserel, (Path *) path);
>>
>>   /* Add paths with pathkeys */
>> - add_paths_with_pathkeys_for_rel(root, baserel, NULL);
>> + add_paths_with_pathkeys_for_rel(root, baserel, NULL, NULL);

> This was my oversight.

No.  IIUC, I think that that would work well as-proposed, but I
changed it as such, for readability.

> So the two patches both look good to me now.

Cool!  I pushed the first patch after polishing it a little bit, so
here is a rebased version of the second patch, in which I modified the
ForeignPath and CustomPath cases in reparameterize_path_by_child() to
reflect the new members fdw_restrictinfo and custom_restrictinfo, for
safety, and tweaked a comment a bit.

Thanks for looking!

Best regards,
Etsuro Fujita

Hi Richard,

On Mon, Jul 31, 2023 at 5:52 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Fri, Jul 28, 2023 at 4:56 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> here is a rebased version of the second patch, in which I modified the
>> ForeignPath and CustomPath cases in reparameterize_path_by_child() to
>> reflect the new members fdw_restrictinfo and custom_restrictinfo, for
>> safety, and tweaked a comment a bit.

> Hmm, it seems that ForeignPath for a foreign join does not support
> parameterized paths for now, as in postgresGetForeignJoinPaths() we have
> this check:
>
>   /*
>    * This code does not work for joins with lateral references, since those
>    * must have parameterized paths, which we don't generate yet.
>    */
>   if (!bms_is_empty(joinrel->lateral_relids))
>       return;
>
> And in create_foreign_join_path() we just set the path.param_info to
> NULL.
>
>   pathnode->path.param_info = NULL;   /* XXX see above */
>
> So I doubt that it's necessary to adjust fdw_restrictinfo in
> reparameterize_path_by_child, because it seems to me that
> fdw_restrictinfo must be empty there.  Maybe we can add an Assert there
> as below:
>
> -        ADJUST_CHILD_ATTRS(fpath->fdw_restrictinfo);
> +
> +        /*
> +         * Parameterized foreign joins are not supported.  So this
> +         * ForeignPath cannot be a foreign join and fdw_restrictinfo
> +         * must be empty.
> +         */
> +        Assert(fpath->fdw_restrictinfo == NIL);
>
> That being said, it's also no harm to handle fdw_restrictinfo in
> reparameterize_path_by_child as the patch does.  So I'm OK we do that
> for safety.

Ok, but maybe my explanation was not good, so let me explain.  The
reason why I modified the code as such is to make the handling of
fdw_restrictinfo consistent with that of fdw_outerpath: we have the
code to reparameterize fdw_outerpath, which should be NULL though, as
we do not currently support parameterized foreign joins.

I modified the code a bit further to use an if-test to avoid a useless
function call, and added/tweaked comments and docs further.  Attached
is a new version of the patch.  I am planning to commit this, if there
are no objections.

Thanks!

Best regards,
Etsuro Fujita

On Tue, Aug 8, 2023 at 6:30 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Tue, Aug 8, 2023 at 4:40 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> I modified the code a bit further to use an if-test to avoid a useless
>> function call, and added/tweaked comments and docs further.  Attached
>> is a new version of the patch.  I am planning to commit this, if there
>> are no objections.

> +1 to the v4 patch.  It looks good to me.

Pushed after some copy-and-paste editing of comments/documents.

Thanks!

Best regards,
Etsuro Fujita

Hi,

On Tue, Aug 15, 2023 at 11:02 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> The commit[1] seems to break some queries in Citus[2], which is an extension which relies on set_join_pathlist_hook.
>
> Although the comment says /*Finally, give extensions a chance to manipulate the path list.*/  we use it to extract
lotsof information about the joins and do the planning based on the information. 
>
> Now,  for some joins where consider_join_pushdown=false, we cannot get the information that we used to get, which
preventsdoing distributed planning for certain queries. 
>
> We wonder if it is possible to allow extensions to access the join info under all circumstances, as it used to be?
Basically,removing the additional check: 
>
> diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
> index 03b3185984..080e76cbe9 100644
> --- a/src/backend/optimizer/path/joinpath.c
> +++ b/src/backend/optimizer/path/joinpath.c
> @@ -349,8 +349,7 @@ add_paths_to_joinrel(PlannerInfo *root,
>         /*
>          * 6. Finally, give extensions a chance to manipulate the path list.
>          */
> -       if (set_join_pathlist_hook &&
> -               consider_join_pushdown)
> +       if (set_join_pathlist_hook)
>                 set_join_pathlist_hook(root, joinrel, outerrel, innerrel,
>                                                            jointype, &extra);

Maybe we could do so by leaving to extensions the decision whether
they replace joins with pseudoconstant clauses, but I am not sure that
that is a good idea, because that would require the authors to modify
and recompile their extensions to fix the issue...  So I fixed the
core side.

I am not familiar with the Citus extension, but such pseudoconstant
clauses are handled within the Citus extension?

Thanks for the report!

Best regards,
Etsuro Fujita

Hi Onder,

On Wed, Aug 16, 2023 at 10:58 PM Önder Kalacı <onderkalaci@gmail.com> wrote:

>> Maybe we could do so by leaving to extensions the decision whether
>> they replace joins with pseudoconstant clauses, but I am not sure that
>> that is a good idea, because that would require the authors to modify
>> and recompile their extensions to fix the issue...

> I think I cannot easily follow this argument. The decision to push down the join
> (or not) doesn't seem to be related to calling set_join_pathlist_hook. It seems like the
> extension should decide what to do with the hook.
>
> That seems the generic theme of the hooks that Postgres provides. For example, the extension
> is allowed to even override the whole planner/executor, and there is no condition that would
> prevent it from happening. In other words, an extension can easily return wrong results with the
> wrong actions taken with the hooks, and that should be responsibility of the extension, not Postgres

>> I am not familiar with the Citus extension, but such pseudoconstant
>> clauses are handled within the Citus extension?

> As I noted earlier, Citus relies on this hook for collecting information about all the joins that Postgres
> knows about, there is nothing specific to pseudoconstants. Some parts of creating the (distributed)
> plan relies on the information gathered from this hook. So, if information about some of the joins
> are not passed to the extension, then the decisions that the extension gives are broken (and as a result
> the queries are broken).

Thanks for the explanation!

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>.

* 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.

BTW: commit 9e9931d2b removed the restriction on the call to the hook
extensions, so you might want to back-patch it.  Though, I think it
would be better if the hook was well implemented from the beginning.

Best regards,
Etsuro Fujita

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

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

Sorry, I hit the send button by mistake.

On Sun, Aug 20, 2023 at 4:34 AM Andres Freund <andres@anarazel.de> wrote:
> On 2023-08-19 20:09:25 +0900, Etsuro Fujita wrote:
> > * 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.

I think you misread the thread; actually, we did an analysis and
applied a fix that would avoid ABI breakage (see the commit message
for 6f80a8d9c).  It turned out that that breaks the Citus extension,
though.

Also, this is not such a change; it is just an optimization
disablement.  Let me explain.  This is the commit message for
e7cb7ee14, which added the hook we are discussing:

    Allow FDWs and custom scan providers to replace joins with scans.

    Foreign data wrappers can use this capability for so-called "join
    pushdown"; that is, instead of executing two separate foreign scans
    and then joining the results locally, they can generate a path which
    performs the join on the remote server and then is scanned locally.
    This commit does not extend postgres_fdw to take advantage of this
    capability; it just provides the infrastructure.

    Custom scan providers can use this in a similar way.  Previously,
    it was only possible for a custom scan provider to scan a single
    relation.  Now, it can scan an entire join tree, provided of course
    that it knows how to produce the same results that the join would
    have produced if executed normally.

As described in the commit message, we assume that extensions use the
hook in a similar way to FDWs; if they do so, the restriction added by
6f80a8d9c just diables them to add paths for join pushdown, making the
planner use paths involving local joins, so any breakage (other than
plan changes from custom joins to local joins) would never happen.

So my question is: does the Citus extension use the hook like this?
(Sorry, I do not fully understand Onder's explanation.)

> > 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.

Yeah, I was thinking that that would be your concern.

Best regards,
Etsuro Fujita

Hi,

Thanks for the detailed explanation!

On Mon, Aug 21, 2023 at 10:34 PM Önder Kalacı <onderkalaci@gmail.com> wrote:

>> As described in the commit message, we assume that extensions use the
>> hook in a similar way to FDWs

> I'm not sure if it is fair to assume that extensions use any hook in any way.

I am not sure either, but as for the hook, I think it is an undeniable
fact that the core system assumes that extensions will use it in that
way.

>> So my question is: does the Citus extension use the hook like this?
>> (Sorry, I do not fully understand Onder's explanation.)

> I haven't gone into detail about how Citus uses this hook, but I don't think we should
> need to explain it. In general, Citus uses many hooks, and many other extensions
> use this specific hook. With minor version upgrades, we haven't seen this kind of
> behavior change before.
>
> In general, Citus relies on this hook for collecting information about joins across
> relations/ctes/subqueries. So, its scope is bigger than a single join for Citus.
>
> The extension assigns a special marker(s) for RTE Relations, and then checks whether
> all relations with these special markers joined transitively across subqueries, such that
> it can decide to pushdown the whole or some parts of the (sub)query.

IIUC, I think that that is going beyond what the hook supports.

> But the bigger issue is that there has usually been a clear line between the extensions and
> the PG itself when it comes to hooks within the minor version upgrades. Sadly, this change
> breaks that line. We wanted to share our worries here and find out what others think.

My understanding is: at least for hooks with intended usages, if an
extension uses them as intended, it is guaranteed that the extension
as-is will work correctly with minor version upgrades; otherwise it is
not necessarily.  I think it is unfortunate that my commit broke the
Citus extension, though.

>> >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.

> I cannot be the one to ask for reverting a commit in PG, but I think doing it would be a
> fair action. We kindly ask those who handle this to think about it.

Reverting the commit would resolve your issue, but re-introduce the
issue mentioned upthread to extensions that use the hook properly, so
I do not think that reverting the commit would be a fair action.

Sorry for the delay.

Best regards,
Etsuro Fujita