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

On Fri, May 31, 2024 at 6:51 PM Japin Li <japinli@hotmail.com> wrote:
> On Fri, 31 May 2024 at 16:22, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > On Fri, May 31, 2024 at 10:12 AM Japin Li <japinli@hotmail.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