Re: postgres_fdw could deparse ArrayCoerceExpr

Hi.

Recently, we were surprised by the following behavior - prepared
statement, selecting data from foreign table with varchar(N) field
couldn't push down "field = ANY($1)" expression, when switched to
generic plan. This looked like shown in the attached patch. Reproducer
is simple:

create extension postgres_fdw;
create server local foreign data wrapper postgres_fdw;
create user MAPPING FOR CURRENT_USER SERVER local;
create table test (c varchar(255));
create foreign table ftest (c varchar(255)) server local options
(table_name 'test');

set plan_cache_mode to force_generic_plan ; -- just for demonstration,
can happen with defautl plan_cache_mode, if planner decides that generic
plan is preferable

prepare s(varchar[]) as select * from ftest where c = any ($1);

explain verbose execute s('{test}');
QUERY PLAN
----------------------------------------------------------------------
Foreign Scan on public.ftest (cost=100.00..143.43 rows=7 width=516)
Output: c
Filter: ((ftest.c)::text = ANY (($1)::text[]))
Remote SQL: SELECT c FROM public.test

The issue is that we need to translate input array type from varchar[]
to text[].

Attaching patch to allow postgres_fdw to deparse such conversion.

--
Best regards,
Alexander Pyhalov,
Postgres Professional

Look like an overlook for me. Apparently no one has encountered this use
case before.

Patch seems good to me with no visible defects. Deparse support was also
added. As well as a
test case. But do we really need copy/paste code for a T_ArrayCoerceExpr
case? To be more specific,
can we "reuse" T_RelabelType case, as it made for T_OpExpr and
T_DistinctExpr?

--
Best regards,
Maxim Orlov.

Maxim Orlov писал(а) 2025-01-24 18:09:
> Look like an overlook for me. Apparently no one has encountered this
> use case before.
>
> Patch seems good to me with no visible defects. Deparse support was
> also added. As well as a
> test case. But do we really need copy/paste code for a
> T_ArrayCoerceExpr case? To be more specific,
> can we "reuse" T_RelabelType case, as it made for T_OpExpr and
> T_DistinctExpr?
>
> --
>

Unfortunately, it's not so simple. We can't just ship type casts to
remote server if we are not sure that local and remote types match. For
example,

CREATE TYPE enum_of_int_like AS enum('1', '2', '3', '4');
CREATE TABLE conversions(id int, d enum_of_int_like);
CREATE FOREIGN TABLE ft_conversions (id int, d char(1))
SERVER loopback options (table_name 'conversions');
INSERT INTO ft_conversions VALUES (1, '1'), (2, '2'), (3, '3'), (4,
'4');

Patched version gives error:

-- Test array type conversion pushdown
SET plan_cache_mode = force_generic_plan;
PREPARE s(varchar[]) AS SELECT count(*) FROM ft_conversions WHERE d =
ANY ($1);
EXPLAIN (VERBOSE, COSTS OFF)
EXECUTE s(ARRAY['1','2']);
QUERY PLAN
---------------------------------------------------------------------------------------------------
Foreign Scan
Output: (count(*))
Relations: Aggregate on (public.ft_conversions)
Remote SQL: SELECT count(*) FROM public.conversions WHERE ((d = ANY
($1::character varying[])))
(4 rows)

EXECUTE s(ARRAY['1','2']);
ERROR: operator does not exist: public.enum_of_int_like = character
varying
HINT: No operator matches the given name and argument types. You might
need to add explicit type casts.
CONTEXT: remote SQL command: SELECT count(*) FROM public.conversions
WHERE ((d = ANY ($1::character varying[])))

Original one does successful local filtering:

PREPARE s(varchar[]) AS SELECT count(*) FROM ft_conversions WHERE d =
ANY ($1);
EXPLAIN (VERBOSE, COSTS OFF)
EXECUTE s(ARRAY['1','2']);
QUERY PLAN
-----------------------------------------------------------
Aggregate
Output: count(*)
-> Foreign Scan on public.ft_conversions
Output: id, d
Filter: (ft_conversions.d = ANY (($1)::bpchar[]))
Remote SQL: SELECT d FROM public.conversions
(6 rows)

EXECUTE s(ARRAY['1','2']);
count
-------
2

--
Best regards,
Alexander Pyhalov,
Postgres Professional

On Mon, 27 Jan 2025 at 09:46, Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>
wrote:

> Unfortunately, it's not so simple. We can't just ship type casts to
> remote server if we are not sure that local and remote types match. For
> example,
>

Yeah, my fault. I've overlooked an "elemexpr" member in "ArrayCoerceExpr"
and erroneously
consider them to have the same structure. Maybe some refactoring may be
done here, but,
obviously, this is not a goal of this patch

--
Best regards,
Maxim Orlov.

So, patch looks good to me. Implements described functionality. Test case
also provided.
I think it's ready to be viewed by a committer.

--
Best regards,
Maxim Orlov.

One important note here. This patch will change cast behaviour in case of
local and foreign types are mismatched.
The problem is if we cannot convert types locally, this does not mean that
it is also true for a foreign wrapped data.
In any case, it's up to the committer to decide whether this change is
needed or not.

--
Best regards,
Maxim Orlov.