Re: ORDER BY pushdowns seem broken in postgres_fdw

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Ronan Dunklau <ronan(dot)dunklau(at)aiven(dot)io>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org, David Zhang <david(dot)zhang(at)highgo(dot)ca>, Zhihong Yu <zyu(at)yugabyte(dot)com>
Subject: Re: ORDER BY pushdowns seem broken in postgres_fdw
Date: 2022-03-30 23:41:37
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Ronan Dunklau <ronan(dot)dunklau(at)aiven(dot)io> writes:
> [ v8-0001-Fix-orderby-handling-in-postgres_fdw.patch ]

I looked through this patch. It's going in the right direction,
but I have a couple of nitpicks:

1. There are still some more places that aren't checking shippability
of the relevant opfamily.

2. The existing usage of find_em_expr_for_rel is fundamentally broken,
because that function will seize on the first EC member that is from the
given rel, whether it's shippable or not. There might be another one
later that is shippable, so this is just the wrong API. It's not like
this function gives us any useful isolation from the details of ECs,
because postgres_fdw is already looking into those elsewhere, notably
in find_em_expr_for_input_target --- which has the same order-sensitivity

I think that instead of doubling down on a wrong API, we should just
take that out and move the logic into postgres_fdw.c. This also has
the advantage of producing a patch that's much safer to backpatch,
because it doesn't rely on the core backend getting updated before is.

So hacking on those two points, and doing some additional cleanup,
led me to the attached v9. (In this patch, the removal of code
from equivclass.c is only meant to be applied to HEAD; we have to
leave the function in place in the back branches for API stability.)

If no objections, I think this is committable.

regards, tom lane

Attachment Content-Type Size
v9-0001-Fix-orderby-handling-in-postgres_fdw.patch text/x-diff 19.9 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2022-03-31 00:01:05 Re: pgsql: Add 'basebackup_to_shell' contrib module.
Previous Message Andres Freund 2022-03-30 23:36:59 Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints