BUG #18467: postgres_fdw (deparser) ignores LimitOption

Hi Onder,

On Wed, May 15, 2024 at 6:07 PM PG Bug reporting form
<noreply@postgresql.org> wrote:

Hi, it seems the same query with `LimitOption` on a heap table and on an
postgres_fdw table pointing to the same heap table is returning different
results.

Thanks for the report! Will look into this next week, because I am
busy with something else this week.

Best regards,
Etsuro Fujita

On Wed, 15 May 2024 at 17:41, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

Hi Onder,

On Wed, May 15, 2024 at 6:07 PM PG Bug reporting form
<noreply@postgresql.org> wrote:

Hi, it seems the same query with `LimitOption` on a heap table and on an
postgres_fdw table pointing to the same heap table is returning different
results.

Thanks for the report! Will look into this next week, because I am
busy with something else this week.

AppendLimitClause() does not check the limitOption, which may be
LIMIT_OPTION_WITH_TIES. It simply uses the LIMIT clause.

Here is a poc patch to verify the above.

--
Regards,
Japin Li

Hi Japin,

Here is a poc patch to verify the above.

The patch looks reasonable to me, it is in line with `get_select_query_def()
<https://github.com/postgres/postgres/blob/8aee330af55d8a759b2b73f5a771d9d34a7b887f/src/backend/utils/adt/ruleutils.c#L5745-L5752&gt;`
.

Though, probably requires a regression test.

Thanks,
Onder

Hi Japin,

On Thu, May 16, 2024 at 1:02 PM Japin Li <japinli@hotmail.com> wrote:

AppendLimitClause() does not check the limitOption, which may be
LIMIT_OPTION_WITH_TIES. It simply uses the LIMIT clause.

Here is a poc patch to verify the above.

Thanks for the patch! Will review.

Best regards,
Etsuro Fujita

On Thu, 16 May 2024 at 17:11, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

Hi Japin,

On Thu, May 16, 2024 at 1:02 PM Japin Li <japinli@hotmail.com> wrote:

AppendLimitClause() does not check the limitOption, which may be
LIMIT_OPTION_WITH_TIES. It simply uses the LIMIT clause.

Here is a poc patch to verify the above.

Thanks for the patch! Will review.

I find that the FETCH FIRST ... ROWS WITH TIES does not support type casting.
For example:

postgres=# SELECT * FROM pg_class ORDER BY relname FETCH FIRST 2::bigint ROWS WITH TIES;
ERROR: syntax error at or near "::"
LINE 1: ...ECT * FROM pg_class ORDER BY relname FETCH FIRST 2::bigint R...
^

I've made a change to support type casting in the select_fetch_first_value.

Please consider reviewing the v2 patch.

--
Regards,
Japin Li

On Thu, 16 May 2024 at 17:02, Önder Kalacı <onderkalaci@gmail.com> wrote:

Hi Japin,

Here is a poc patch to verify the above.

The patch looks reasonable to me, it is in line with `get_select_query_def()
<https://github.com/postgres/postgres/blob/8aee330af55d8a759b2b73f5a771d9d34a7b887f/src/backend/utils/adt/ruleutils.c#L5745-L5752&gt;`
.

Though, probably requires a regression test.

Yeah, add the regression test in v2 patch [1]/messages/by-id/ME3P282MB31666446BF8A0C0F7A02D24CB6ED2@ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM.

[1]: /messages/by-id/ME3P282MB31666446BF8A0C0F7A02D24CB6ED2@ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM

--
Regards,
Japin Li

On 2024-May-16, Japin Li wrote:

I find that the FETCH FIRST ... ROWS WITH TIES does not support type casting.
For example:

postgres=# SELECT * FROM pg_class ORDER BY relname FETCH FIRST 2::bigint ROWS WITH TIES;
ERROR: syntax error at or near "::"
LINE 1: ...ECT * FROM pg_class ORDER BY relname FETCH FIRST 2::bigint R...
^

Why do you need this? The standard says

<fetch first clause> ::= FETCH { FIRST | NEXT } [ <fetch first quantity> ] { ROW | ROWS } { ONLY | WITH TIES }
<fetch first quantity> ::= <fetch first row count> | <fetch first percentage>
<offset row count> ::= <simple value specification>
<fetch first row count> ::= <simple value specification>

which doesn't seem to leave room for a cast.

I didn't try super extensively, but this works:
select 1 from pg_class fetch first 281474976710656 rows only;
so the count is already not restricted to be an int32 value.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

On Thu, 16 May 2024 at 23:27, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2024-May-16, Japin Li wrote:

I find that the FETCH FIRST ... ROWS WITH TIES does not support type casting.
For example:

postgres=# SELECT * FROM pg_class ORDER BY relname FETCH FIRST 2::bigint ROWS WITH TIES;
ERROR: syntax error at or near "::"
LINE 1: ...ECT * FROM pg_class ORDER BY relname FETCH FIRST 2::bigint R...
^

Why do you need this? The standard says

The limitCount is stored as bigint and deparseConst() will automatically append
'::bigint'. So I do the typecast.

<fetch first clause> ::= FETCH { FIRST | NEXT } [ <fetch first quantity> ] { ROW | ROWS } { ONLY | WITH TIES }
<fetch first quantity> ::= <fetch first row count> | <fetch first percentage>
<offset row count> ::= <simple value specification>
<fetch first row count> ::= <simple value specification>

which doesn't seem to leave room for a cast.

I didn't try super extensively, but this works:
select 1 from pg_class fetch first 281474976710656 rows only;
so the count is already not restricted to be an int32 value.

Since it is of type int, why should it be stored as bigint?

--
Regards,
Japin Li

On Thu, 16 May 2024 at 23:27, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2024-May-16, Japin Li wrote:

I find that the FETCH FIRST ... ROWS WITH TIES does not support type casting.
For example:

postgres=# SELECT * FROM pg_class ORDER BY relname FETCH FIRST 2::bigint ROWS WITH TIES;
ERROR: syntax error at or near "::"
LINE 1: ...ECT * FROM pg_class ORDER BY relname FETCH FIRST 2::bigint R...
^

Why do you need this? The standard says

<fetch first clause> ::= FETCH { FIRST | NEXT } [ <fetch first quantity> ] { ROW | ROWS } { ONLY | WITH TIES }
<fetch first quantity> ::= <fetch first row count> | <fetch first percentage>
<offset row count> ::= <simple value specification>
<fetch first row count> ::= <simple value specification>

which doesn't seem to leave room for a cast.

I didn't try super extensively, but this works:
select 1 from pg_class fetch first 281474976710656 rows only;
so the count is already not restricted to be an int32 value.

After some dig in, I find transformLimitClause() has the following comments:

* Note: as of Postgres 8.2, LIMIT expressions are expected to yield int8,
* rather than int4 as before.

So the deparseConst() append '::bigint' typecast, same as get_rule_expr().

--
Regards,
Japin Li

On 2024-May-17, Japin Li wrote:

After some dig in, I find transformLimitClause() has the following comments:

* Note: as of Postgres 8.2, LIMIT expressions are expected to yield int8,
* rather than int4 as before.

So the deparseConst() append '::bigint' typecast, same as get_rule_expr().

Hmm, probably the answer is not to use deparseExpr in
appendLimitClause() then. I mean, AFAICS this is a postgres_fdw
problem, not a core parser problem.

Not sure if this is workable, but maybe test if the value is a constant,
and use FETCH FIRST if so, otherwise fall back to using LIMIT. (It's
not clear if this would handle FETCH FIRST +123123123123 ROWS WITH TIES
correctly though -- worth checking).

Oh, I see we never got back to adding FETCH FIRST / PERCENT. We had a
patch seemingly almost ready for that ...

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2024-May-16, Japin Li wrote:

I find that the FETCH FIRST ... ROWS WITH TIES does not support type casting.

Why do you need this? The standard says

<fetch first clause> ::= FETCH { FIRST | NEXT } [ <fetch first quantity> ] { ROW | ROWS } { ONLY | WITH TIES }
<fetch first quantity> ::= <fetch first row count> | <fetch first percentage>
<offset row count> ::= <simple value specification>
<fetch first row count> ::= <simple value specification>
which doesn't seem to leave room for a cast.

Exactly. "LIMIT 42::int8" is accepted, but the equivalent
with FETCH is not, so that there's a hazard of the remote
rejecting the query depending on how deparseExpr chooses
to print the limit expression. Ordinarily it hasn't the
slightest hesitation about affixing casts to constants,
so I suspect there's a live problem there.

I think that this approach to a fix might be the wrong thing
altogether, and that it might be better to refuse to send WITH_TIES
clauses to the remote at all. There are two things that are scaring
me about that:

1. The remote might have a different idea of equality than we do,
leading to different results. Admittedly that's a little bit
far-fetched, but with things like nondeterministic collations floating
around, it's by no means impossible.

2. If the remote is older than v13 the query will fail entirely
for lack of support for WITH TIES.

At the very least we need a remote-version check before deciding
that it'll be OK to ship such a clause. But if there are other
gotchas, as it seems there are, let's just cut our losses and
not do it.

regards, tom lane

On Fri, 17 May 2024 at 00:58, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2024-May-16, Japin Li wrote:

I find that the FETCH FIRST ... ROWS WITH TIES does not support type casting.

Why do you need this? The standard says

<fetch first clause> ::= FETCH { FIRST | NEXT } [ <fetch first quantity> ] { ROW | ROWS } { ONLY | WITH TIES }
<fetch first quantity> ::= <fetch first row count> | <fetch first percentage>
<offset row count> ::= <simple value specification>
<fetch first row count> ::= <simple value specification>
which doesn't seem to leave room for a cast.

Exactly. "LIMIT 42::int8" is accepted, but the equivalent
with FETCH is not, so that there's a hazard of the remote
rejecting the query depending on how deparseExpr chooses
to print the limit expression. Ordinarily it hasn't the
slightest hesitation about affixing casts to constants,
so I suspect there's a live problem there.

I think that this approach to a fix might be the wrong thing
altogether, and that it might be better to refuse to send WITH_TIES
clauses to the remote at all. There are two things that are scaring
me about that:

1. The remote might have a different idea of equality than we do,
leading to different results. Admittedly that's a little bit
far-fetched, but with things like nondeterministic collations floating
around, it's by no means impossible.

2. If the remote is older than v13 the query will fail entirely
for lack of support for WITH TIES.

I didn't take this into consideration previously.

At the very least we need a remote-version check before deciding
that it'll be OK to ship such a clause. But if there are other
gotchas, as it seems there are, let's just cut our losses and
not do it.

Try to disable the push-down FETCH FIRST clause as you suggested.

--
Regards,
Japin Li

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?

+ /*

+ * 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;
+

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

Thanks,
Onder

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

Hi Japin,

Thanks, the patch looks good to me.

However, as a non-native English speaker, it might not be accurate.

Feel free to modify it.

I'm also not good at wording, but I have a minor suggestions like the
following :

/*
* Also, the FETCH FIRST/NEXT ... ROW/ROWS WITH TIES clause cannot be
pushed down
* because:
* a) The remote system may have a different understanding of equality,
which can
* result in varying results, such as non-deterministic collations.
* b) We do not have knowledge of the remote server's version
* as this clause is only supported for PG13 and above.
*/

Thanks,
Onder

On Thu, 23 May 2024 at 14:54, Önder Kalacı <onderkalaci@gmail.com> wrote:

Hi Japin,

Thanks, the patch looks good to me.

However, as a non-native English speaker, it might not be accurate.

Feel free to modify it.

I'm also not good at wording, but I have a minor suggestions like the
following :

/*
* Also, the FETCH FIRST/NEXT ... ROW/ROWS WITH TIES clause cannot be
pushed down
* because:
* a) The remote system may have a different understanding of equality,
which can
* result in varying results, such as non-deterministic collations.
* b) We do not have knowledge of the remote server's version
* as this clause is only supported for PG13 and above.
*/

Thanks for your review! Fixed in v5 patch.

--
Regards,
Japin Li

Hi,

On Thu, May 23, 2024 at 11:30 PM Japin Li <japinli@hotmail.com> wrote:

On Thu, 23 May 2024 at 14:54, Önder Kalacı <onderkalaci@gmail.com> wrote:

I'm also not good at wording, but I have a minor suggestions like the
following :

/*
* Also, the FETCH FIRST/NEXT ... ROW/ROWS WITH TIES clause cannot be
pushed down
* because:
* a) The remote system may have a different understanding of equality,
which can
* result in varying results, such as non-deterministic collations.
* b) We do not have knowledge of the remote server's version
* as this clause is only supported for PG13 and above.
*/

Thanks for your review! Fixed in v5 patch.

I think it is reasonable to refuse to send WITH TIES, but I am
confused about the comments above. Do we really need to care about a)
here in add_foreign_final_paths()? If the query has WITH TIES, 1) it
must have ORDER BY as well, which determines what 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, if getting here, we can consider that WITH TIES is also
safe to push down (if the remote is v13 or later). No?

Anyway, thank you for working on this issue!

Best regards,
Etsuro Fujita

On Fri, 24 May 2024 at 16:32, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

Hi,

On Thu, May 23, 2024 at 11:30 PM Japin Li <japinli@hotmail.com> wrote:

On Thu, 23 May 2024 at 14:54, Önder Kalacı <onderkalaci@gmail.com> wrote:

I'm also not good at wording, but I have a minor suggestions like the
following :

/*
* Also, the FETCH FIRST/NEXT ... ROW/ROWS WITH TIES clause cannot be
pushed down
* because:
* a) The remote system may have a different understanding of equality,
which can
* result in varying results, such as non-deterministic collations.
* b) We do not have knowledge of the remote server's version
* as this clause is only supported for PG13 and above.
*/

Thanks for your review! Fixed in v5 patch.

I think it is reasonable to refuse to send WITH TIES, but I am
confused about the comments above. Do we really need to care about a)
here in add_foreign_final_paths()? If the query has WITH TIES, 1) it
must have ORDER BY as well, which determines what 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, if getting here, we can consider that WITH TIES is also
safe to push down (if the remote is v13 or later). No?

Anyway, thank you for working on this issue!

Sorry for the late reply. I'm not familiar with this. However, after some
tests, the COLLATE may influence the result; see the example below.

[local]:535513 postgres=# CREATE TABLE t01 (a text);
CREATE TABLE
[local]:535513 postgres=# CREATE FOREIGN TABLE ft01 (a text) SERVER loopback OPTIONS (table_name 't01');
CREATE FOREIGN TABLE
[local]:535513 postgres=# SELECT * FROM ft01 ORDER BY a COLLATE "en_US" FETCH FIRST 2 ROWS WITH TIES;
a
-------
hello
hello
hello
(3 rows)

[local]:535513 postgres=# EXPLAIN (verbose) SELECT * FROM ft01 ORDER BY a COLLATE "en_US" FETCH FIRST 2 ROWS WITH TIES;
QUERY PLAN
-----------------------------------------------------------------------------------
Limit (cost=446.26..446.27 rows=2 width=64)
Output: a, ((a)::text)
-> Sort (cost=446.26..449.92 rows=1462 width=64)
Output: a, ((a)::text)
Sort Key: ft01.a COLLATE "en_US"
-> Foreign Scan on public.ft01 (cost=100.00..431.64 rows=1462 width=64)
Output: a, a
Remote SQL: SELECT a FROM public.t01
(8 rows)

[local]:535513 postgres=# SELECT * FROM ft01 ORDER BY a FETCH FIRST 2 ROWS WITH TIES;
a
-------
hello
hello
(2 rows)

[local]:535513 postgres=# EXPLAIN (verbose) SELECT * FROM ft01 ORDER BY a FETCH FIRST 2 ROWS WITH TIES;
QUERY PLAN
----------------------------------------------------------------------------------
Foreign Scan on public.ft01 (cost=100.00..100.44 rows=2 width=32)
Output: a
Remote SQL: SELECT a FROM public.t01 ORDER BY a ASC NULLS LAST LIMIT 2::bigint
(3 rows)

--
Regards,
Japin Li

Hi,

and 2) ORDER BY must already

have been determined to be safe to push down before we get here.

When I read the code, the decision for that seems to happen in the next
line where this patch proposes to modify:

/*

* Also, the LIMIT/OFFSET cannot be pushed down, if their expressions are
* not safe to remote.
*/
if (!is_foreign_expr(root, input_rel, (Expr *) parse->limitOffset) ||
!is_foreign_expr(root, input_rel, (Expr *) parse->limitCount))
return;

Hence, I thought it is OK to add this new check to this place.

So

in that case, if getting here, we can consider that WITH TIES is also
safe to push down (if the remote is v13 or later).

See Tom's response here on why it might not be a good idea to pushdown
WITH TIES:
/messages/by-id/2114796.1715878709@sss.pgh.pa.us

I think, at least not with this patch, this patch is like a bug-fix. If
intended, it should be possible to
pushdown WITH TIES with a follow-up patch?

Thanks,
Onder