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

On Mon, 20 May 2024 at 17:08, Önder Kalacı <onderkalaci@gmail.com> wrote:
> Hi,
>
>>
>> altogether, and that it might be better to refuse to send WITH_TIES
>>> clauses to the remote at all.  I think that this approach to a fix might
>>> be the wrong thing
>>
>>
>>
> I didn't take this into consideration previously.
>
>
> I think I confused many people with my first bug-report, where I mentioned
> about deparser. Sorry about that.
>
> Reading Tom's response, I think he is right, it sounds simpler & better to
> refuse to send WITH_TIES at all. Especially if we consider this as a
> candidate
> for backport to earlier versions. I think supporting pushdown of WITH_TIES
> can probably be considered as a new feature, and deserves its own patch &
> discussion.
>
>> +SELECT * FROM ft1 t1 ORDER BY t1.c2 FETCH FIRST 2 ROWS WITH TIES;
>
>
> I think it is excessive that the query returns 100 rows. Can we add few
> filters, like
> a filter (c5 = `Thu Jan 01 00:00:00 1970` AND c3 > '00500') or such that we
> only have
> few rows to show in the output?

Fixed.

>
> + /*
>> + * Also, the FETCH FIRST/NEXT ... ROW/ROWS WITH TIES cannot be pushed down
>> + * due to potential lack of support for this clause on the remote.
>> + */
>
>
> Would be nice to mention potential risks due to collation-incompatibilities
> in this comment.
> I feel like that is probably more important than the lack of WITH TIES
> support. For example,
> in a few years, someone might decide to remove this check by assuming that
> it is only
> added as PG 13 would not be officially supported by the community (e.g.,
> end of life).
>
> + if (parse->limitOption == LIMIT_OPTION_WITH_TIES)
>> + return;
>> +
>

Sorry, I forgot the first reason that Tom mentioned. Fixed.
However, as a non-native English speaker, it might not be accurate.
Feel free to modify it.

>
> Other than that, the check you added looks like a reasonable place to add.

--
Regards,
Japin Li