Re: BUG #18467: postgres_fdw (deparser) ignores LimitOption

On Fri, May 31, 2024 at 6:51 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
> On Fri, 31 May 2024 at 16:22, Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> > On Fri, May 31, 2024 at 10:12 AM Japin Li <japinli(at)hotmail(dot)com> wrote:
> >> I think I understand what you mean. We can ensure that the ORDER BY can be
> >> safely pushed down if we are in add_foreign_final_paths(). The reason the
> >> FETCH clause cannot be pushed down is only because the remote may not
> >> support it, right?
> >
> > Yeah, I think so;

One thing I forgot to mention is the parsing/deparsing issue in
core/postgres_fdw discussed upthread. From the postgres_fdw side, I
think a simple workaround for that is to enclose
constants-with-the-cast in parentheses.

> > for the next person, I would like to propose to
> > update the comments proposed upthread to something like this:
> >
> > /*
> > * If the query uses FETCH FIRST .. WITH TIES, 1) it must have ORDER BY as
> > * well, which is used to determine which additional rows tie for the last
> > * place in the result set, and 2) ORDER BY must already have been
> > * determined to be safe to push down before we get here. So in that case
> > * the FETCH clause is safe to push down with ORDER BY if the remote
> > * server is v13 or later; but if not, the remote query will fail entirely
> > * for lack of support for it. Since we do not currently have a way to do
> > * a remote-version check (without accessing the remote server), disable
> > * pushing it for now.
> > */

> Thanks for the rewording! LGTM.

Done in the attached.

Another thing I noticed is this:

+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE t1.c3 >
'00960' ORDER BY t1.c2 FETCH FIRST 2 ROWS WITH TIES;
+ QUERY PLAN
+---------------------------------------------------------------------------------------------------------------------------------
+ Limit
+ Output: c1, c2, c3, c4, c5, c6, c7, c8
+ -> Foreign Scan on public.ft1 t1
+ Output: c1, c2, c3, c4, c5, c6, c7, c8
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S
1"."T 1" WHERE ((c3 > '00960')) ORDER BY c2 ASC NULLS LAST
+(5 rows)
+
+SELECT * FROM ft1 t1 WHERE t1.c3 > '00960' ORDER BY t1.c2 FETCH FIRST
2 ROWS WITH TIES;
+ c1 | c2 | c3 | c4 | c5
| c6 | c7 | c8
+------+----+-------+------------------------------+--------------------------+----+------------+-----
+ 970 | 0 | 00970 | Thu Mar 12 00:00:00 1970 PST | Thu Mar 12
00:00:00 1970 | 0 | 0 | foo
+ 1000 | 0 | 01000 | Thu Jan 01 00:00:00 1970 PST | Thu Jan 01
00:00:00 1970 | 0 | 0 | foo
+ 990 | 0 | 00990 | Wed Apr 01 00:00:00 1970 PST | Wed Apr 01
00:00:00 1970 | 0 | 0 | foo
+ 980 | 0 | 00980 | Sun Mar 22 00:00:00 1970 PST | Sun Mar 22
00:00:00 1970 | 0 | 0 | foo
+(4 rows)

* The filter uses column c3, which is of type text, so I think the
test can produce different results when running it against an
already-installed server with non-C locale settings such as the
--icu-locale and --icu-rules options in initdb [1].
* IIUC, the result ordering depends on the implementation of tuple
sorting; if we made some changes to the implementation, the result
ordering might change as well.

Admittedly, these are far-fetched, but better safe than sorry; I
slightly modified the test for better stability.

If there are no objections, I will push the patch.

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/docs/current/regress-evaluation.html#REGRESS-EVALUATION-ORDERING-DIFFERENCES