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

Hi,

Thanks for the detailed explanation!

On Mon, Aug 21, 2023 at 10:34 PM Önder Kalacı <onderkalaci(at)gmail(dot)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