Horton

Thread

Commits

  1. postgres_fdw: suppress casts on constants in limited cases.

  1. [PATCH] postgres-fdw: column option to override foreign types

    Dian Fay <dian.m.fay@gmail.com> — 2021-03-01T07:24:01Z

    Full use of a custom data type with postgres_fdw currently requires the
type be maintained in both the local and remote databases. `CREATE
FOREIGN TABLE` does not check declared types against the remote table,
but declaring e.g. a remote enum to be local text works only partway, as
seen here. A simple select query against alpha_items returns the enum
values as text; however, *filtering* on the column yields an error.

create database alpha;
create database beta;

\c alpha

create type itemtype as enum ('one', 'two', 'three');
create table items (
  id serial not null primary key,
  type itemtype not null
);
insert into items (type) values ('one'), ('one'), ('two');

\c beta

create extension postgres_fdw;
create server alpha foreign data wrapper postgres_fdw options (dbname 'alpha', host 'localhost', port '5432');
create user mapping for postgres server alpha options (user 'postgres');

create foreign table alpha_items (
  id int,
  type text
) server alpha options (table_name 'items');
select * from alpha_items; -- ok
select * from alpha_items where type = 'one';

ERROR:  operator does not exist: public.itemtype = text
HINT:  No operator matches the given name and argument types. You might need to add explicit type casts.
CONTEXT:  remote SQL command: SELECT id, type FROM public.items WHERE ((type = 'one'::text))

The attached changeset adds a new boolean option for postgres_fdw
foreign table columns, `use_local_type`. When true, ColumnRefs for the
relevant attribute will be deparsed with a cast to the type defined in
`CREATE FOREIGN TABLE`.

create foreign table alpha_items (
  id int,
  type text options (use_local_type 'true')
) server alpha options (table_name 'items');
select * from alpha_items where type = 'one'; -- succeeds

This builds and checks, with a new regression test and documentation.

  2. Re: [PATCH] postgres-fdw: column option to override foreign types

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2021-03-02T11:50:10Z

    On Mon, Mar 1, 2021 at 12:59 PM Dian M Fay <dian.m.fay@gmail.com> wrote:
>
> Full use of a custom data type with postgres_fdw currently requires the
> type be maintained in both the local and remote databases. `CREATE
> FOREIGN TABLE` does not check declared types against the remote table,
> but declaring e.g. a remote enum to be local text works only partway, as
> seen here. A simple select query against alpha_items returns the enum
> values as text; however, *filtering* on the column yields an error.
>
> create database alpha;
> create database beta;
>
> \c alpha
>
> create type itemtype as enum ('one', 'two', 'three');
> create table items (
>   id serial not null primary key,
>   type itemtype not null
> );
> insert into items (type) values ('one'), ('one'), ('two');
>
> \c beta
>
> create extension postgres_fdw;
> create server alpha foreign data wrapper postgres_fdw options (dbname 'alpha', host 'localhost', port '5432');
> create user mapping for postgres server alpha options (user 'postgres');
>
> create foreign table alpha_items (
>   id int,
>   type text
> ) server alpha options (table_name 'items');

postgres_fdw assumes that the local type declared is semantically same
as the remote type. Ideally the enum should also be declared locally
and used to declare type's datatype. See how to handle UDTs in
postgres_fdw at
https://stackoverflow.com/questions/37734170/can-the-foreign-data-wrapper-fdw-postgres-handle-the-geometry-data-type-of-postg

--
Best Wishes,
Ashutosh Bapat

  3. Re: [PATCH] postgres-fdw: column option to override foreign types

    Dian Fay <dian.m.fay@gmail.com> — 2021-03-02T13:34:50Z

    On Tue Mar 2, 2021 at 6:50 AM EST, Ashutosh Bapat wrote:
> On Mon, Mar 1, 2021 at 12:59 PM Dian M Fay <dian.m.fay@gmail.com> wrote:
> >
> > Full use of a custom data type with postgres_fdw currently requires the
> > type be maintained in both the local and remote databases. `CREATE
> > FOREIGN TABLE` does not check declared types against the remote table,
> > but declaring e.g. a remote enum to be local text works only partway, as
> > seen here. A simple select query against alpha_items returns the enum
> > values as text; however, *filtering* on the column yields an error.
>
> postgres_fdw assumes that the local type declared is semantically same
> as the remote type. Ideally the enum should also be declared locally
> and used to declare type's datatype. See how to handle UDTs in
> postgres_fdw at
> https://stackoverflow.com/questions/37734170/can-the-foreign-data-wrapper-fdw-postgres-handle-the-geometry-data-type-of-postg

I'm aware, and the reason for this change is that I think it's annoying
to declare and maintain the type on the local server for the sole
purpose of accommodating a read-only foreign table that effectively
treats it like text anyway. The real scenario that prompted it is a
tickets table with status, priority, category, etc. enums. We don't have
plans to modify them any time soon, but if we do it's got to be
coordinated and deployed across two databases, all so we can use what
might as well be a text column in a single WHERE clause. Since foreign
tables can be defined over subsets of columns, reordered, and names
changed, a little opt-in flexibility with types too doesn't seem
misplaced.

Note that currently, postgres_fdw will strip casts on the WHERE column:
`where type::text = 'one'` becomes `where ((type = 'one'::text))` (the
value is cast separately). Making it respect those is another option,
but I thought including it in column configuration would be less
surprising to users who aren't aware of the difference between the local
and remote tables.

  4. Re: [PATCH] postgres-fdw: column option to override foreign types

    Georgios <gkokolatos@protonmail.com> — 2021-03-04T14:28:46Z

    The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

Hi,

Thanks for the patch.

I am afraid I will have to :-1: this patch. Of course it is possible that I am wrong,
so please correct me if you, or any other reviewers, think so.

The problem that is intended to be solved, upon closer inspection seems to be a
conscious design decision rather than a problem. Even if I am wrong there, I am
not certain that the proposed patch covers all the bases with respect to collations,
build-in types, shipability etc for simple expressions, and covers any of more
complicated expressions all together. 

As for the scenario which prompted the patch, you wrote, quote:

The real scenario that prompted it is a
tickets table with status, priority, category, etc. enums. We don't have
plans to modify them any time soon, but if we do it's got to be
coordinated and deployed across two databases, all so we can use what
might as well be a text column in a single WHERE clause. Since foreign
tables can be defined over subsets of columns, reordered, and names
changed, a little opt-in flexibility with types too doesn't seem
misplaced. 

end quote.

I will add that creating a view on the remote server with type flexibility that
you wish and then create foreign tables against the view, might address your
problem.

Respectfully,
//Georgios

  5. Re: [PATCH] postgres-fdw: column option to override foreign types

    David Steele <david@pgmasters.net> — 2021-03-04T16:48:48Z

    On 3/4/21 9:28 AM, Georgios Kokolatos wrote:
> 
> I am afraid I will have to :-1: this patch. Of course it is possible that I am wrong,
> so please correct me if you, or any other reviewers, think so.

I'm inclined to agree and it seems like a view on the source server is a 
good compromise and eliminates the maintenance concerns.

I'm going to mark this as Waiting on Author for now, but will close it 
on March 11 if there are no arguments in support.

Dian, perhaps you have another angle you'd like to try?

Regards,
-- 
-David
david@pgmasters.net

  6. Re: [PATCH] postgres-fdw: column option to override foreign types

    Dian Fay <dian.m.fay@gmail.com> — 2021-03-04T19:29:47Z

    On Thu Mar 4, 2021 at 9:28 AM EST, Georgios Kokolatos wrote:
> I am afraid I will have to :-1: this patch. Of course it is possible
> that I am wrong,
> so please correct me if you, or any other reviewers, think so.
>
> The problem that is intended to be solved, upon closer inspection
> seems
> to be a
> conscious design decision rather than a problem. Even if I am wrong
> there, I am
> not certain that the proposed patch covers all the bases with respect
> to
> collations,
> build-in types, shipability etc for simple expressions, and covers any
> of more
> complicated expressions all together.

Thanks for reviewing it!

I see room for interpretation in the design here, although I have
admittedly not been looking at it for very long. `CREATE/ALTER FOREIGN
TABLE` take the user at their word about types, which only map 1:1 for a
foreign Postgres server anyway. In make_tuple_from_result_row, incoming
values start as strings until they're converted to their target types --
again, with no guarantee that those types match those on the remote
server. The docs recommend types match exactly and note the sorts of
things that can go wrong, but there's no enforcement; either what you've
cooked up works or it doesn't. And in fact, declaring local text for a
remote enum seems to work quite well.... right up until you try to
reference it in the `WHERE` clause.

Enum::text seems like a safe and potentially standardizable case for
postgres_fdw. As implemented, the patch goes beyond that, but it's
opt-in and the docs already warn about consequences. I haven't tested it
across collations, but right now that seems like something to look into
if the idea survives the next few messages.

> I will add that creating a view on the remote server with type
> flexibility that
> you wish and then create foreign tables against the view, might
> address
> your
> problem.

A view would address the immediate issue of the types, but itself
requires additional maintenance if/when the underlying table's schema
changes (even `SELECT *` is expanded into the current column definitions
at creation). I think it's better than copying the types, because it
moves the extra work of keeping local and remote synchronized to a
*table* modification as opposed to a *type* modification, in which
latter case it's much easier to forget about dependents. But I'd prefer
to avoid extra work anywhere!

  7. Re: [PATCH] postgres-fdw: column option to override foreign types

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-04T21:28:49Z

    "Dian M Fay" <dian.m.fay@gmail.com> writes:
> On Thu Mar 4, 2021 at 9:28 AM EST, Georgios Kokolatos wrote:
>> I am afraid I will have to :-1: this patch.

> I see room for interpretation in the design here, although I have
> admittedly not been looking at it for very long. `CREATE/ALTER FOREIGN
> TABLE` take the user at their word about types, which only map 1:1 for a
> foreign Postgres server anyway.

Right.

> In make_tuple_from_result_row, incoming
> values start as strings until they're converted to their target types --
> again, with no guarantee that those types match those on the remote
> server.

The data conversion itself provides a little bit of security --- for
instance, converting 'foobar' to int or timestamp will fail.  It's
not bulletproof, but on the other hand there are indeed situations
where you don't want to declare the column locally with exactly the
type the remote server is using, so trying to be bulletproof would
be counterproductive.

I am not, however, any more impressed than the other respondents with
the solution you've proposed.  For one thing, this can only help if
the local type is known to the remote server, which seems to eliminate
fifty per cent of the use-cases for intentional differences in type.
(That is, isn't it equally as plausible that the local type is an
enum you didn't bother making on the remote side?)  But a bigger issue
is that shipping
	WHERE foreigncol::text = 'one'::text
to the remote server is not a nice solution even if it works.  It will,
for example, defeat use of a normal index on foreigncol.  It'd likely
be just as inefficient for remote joins.

What'd be better, if we could do it, is to ship the clause in
the form
	WHERE foreigncol = 'one'
that is, instead of plastering a cast on the Var, try to not put
any explicit cast on the constant.  That fixes your original use
case better than what you've proposed, and I think it might be
possible to do it unconditionally instead of needing a hacky
column property to enable it.  The reason this could be okay
is that it seems reasonable for postgres_fdw to rely on the
core parser's heuristic that an unknown-type literal is the
same type as what it's being compared to.  So, if we are trying
to deparse something of the form "foreigncol operator constant",
and the foreigncol and constant are of the same type locally,
we could leave off the cast on the constant.  (There might need
to be some restrictions about the operator taking those types
natively with no cast, not sure; also this doesn't apply to
constants that are going to be printed as non-string literals.)

Slipping this heuristic into the code structure of deparse.c
might be rather messy, though.  I've not looked at just how
to implement it.

			regards, tom lane

  8. Re: [PATCH] postgres-fdw: column option to override foreign types

    Dian Fay <dian.m.fay@gmail.com> — 2021-03-07T07:37:53Z

    > What'd be better, if we could do it, is to ship the clause in
> the form
> WHERE foreigncol = 'one'
> that is, instead of plastering a cast on the Var, try to not put
> any explicit cast on the constant. That fixes your original use
> case better than what you've proposed, and I think it might be
> possible to do it unconditionally instead of needing a hacky
> column property to enable it. The reason this could be okay
> is that it seems reasonable for postgres_fdw to rely on the
> core parser's heuristic that an unknown-type literal is the
> same type as what it's being compared to. So, if we are trying
> to deparse something of the form "foreigncol operator constant",
> and the foreigncol and constant are of the same type locally,
> we could leave off the cast on the constant. (There might need
> to be some restrictions about the operator taking those types
> natively with no cast, not sure; also this doesn't apply to
> constants that are going to be printed as non-string literals.)
>
> Slipping this heuristic into the code structure of deparse.c
> might be rather messy, though. I've not looked at just how
> to implement it.

This doesn't look too bad from here, at least so far. The attached
change adds a new const_showtype field to the deparse_expr_cxt, and
passes that instead of the hardcoded 0 to deparseConst. deparseOpExpr
modifies const_showtype if both sides of a binary operation are text,
and resets it to 0 after the recursion.

I restricted it to text-only after seeing a regression test fail: while
deparsing `percentile_cont(c2/10::numeric)`, c2, an integer column, is a
FuncExpr with a numeric return type. That matches the numeric 10, and
without the explicit cast, integer-division-related havoc ensues. I
don't know why it's a FuncExpr, and I don't know why it's not an int,
but the constant is definitely a non-string, in any case.

In the course of testing, I discovered that the @@ text-search operator
works against textified enums on my stock 13.1 server (a "proper" enum
column yields "operator does not exist"). I'm rather wary of actually
trying to depend on that behavior, although it seems probably-safe in
the same character set and collation.

  9. Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)

    Dian Fay <dian.m.fay@gmail.com> — 2021-08-05T02:38:24Z

    On Sun Mar 7, 2021 at 2:37 AM EST, Dian M Fay wrote:
> > What'd be better, if we could do it, is to ship the clause in
> > the form
> > WHERE foreigncol = 'one'
> > that is, instead of plastering a cast on the Var, try to not put
> > any explicit cast on the constant. That fixes your original use
> > case better than what you've proposed, and I think it might be
> > possible to do it unconditionally instead of needing a hacky
> > column property to enable it. The reason this could be okay
> > is that it seems reasonable for postgres_fdw to rely on the
> > core parser's heuristic that an unknown-type literal is the
> > same type as what it's being compared to. So, if we are trying
> > to deparse something of the form "foreigncol operator constant",
> > and the foreigncol and constant are of the same type locally,
> > we could leave off the cast on the constant. (There might need
> > to be some restrictions about the operator taking those types
> > natively with no cast, not sure; also this doesn't apply to
> > constants that are going to be printed as non-string literals.)
> >
> > Slipping this heuristic into the code structure of deparse.c
> > might be rather messy, though. I've not looked at just how
> > to implement it.
>
> This doesn't look too bad from here, at least so far. The attached
> change adds a new const_showtype field to the deparse_expr_cxt, and
> passes that instead of the hardcoded 0 to deparseConst. deparseOpExpr
> modifies const_showtype if both sides of a binary operation are text,
> and resets it to 0 after the recursion.
>
> I restricted it to text-only after seeing a regression test fail: while
> deparsing `percentile_cont(c2/10::numeric)`, c2, an integer column, is a
> FuncExpr with a numeric return type. That matches the numeric 10, and
> without the explicit cast, integer-division-related havoc ensues. I
> don't know why it's a FuncExpr, and I don't know why it's not an int,
> but the constant is definitely a non-string, in any case.
>
> In the course of testing, I discovered that the @@ text-search operator
> works against textified enums on my stock 13.1 server (a "proper" enum
> column yields "operator does not exist"). I'm rather wary of actually
> trying to depend on that behavior, although it seems probably-safe in
> the same character set and collation.

hello again! My second version of this change (suppressing the cast
entirely as Tom suggested) seemed to slip under the radar back in March
and then other matters intervened. I'm still interested in making it
happen, though, and now that we're out of another commitfest it seems
like a good time to bring it back up. Here's a rebased patch to start.

  10. Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-09-05T22:43:45Z

    "Dian M Fay" <dian.m.fay@gmail.com> writes:
> [ 0001-Suppress-explicit-casts-of-text-constants-in-postgre.patch ]

I took a quick look at this.  The restriction to type text seems like
very obviously a hack rather than something we actually want; wouldn't
it mean we fail to act in a large fraction of the cases where we'd
like to suppress the cast?

A second problem is that I don't think the business with a const_showtype
context field is safe at all.  As you've implemented it here, it would
affect the entire RHS tree, including constants far down inside complex
expressions that have nothing to do with the top-level semantics.
(I didn't look closely, but I wonder if the regression failure you
mentioned is associated with that.)

I think that we only want to suppress the cast in cases where
(1) the constant is directly an operand of the operator we're
expecting the remote parser to use its same-type heuristic for, and
(2) the constant will be deparsed as a string literal.  (If it's
deparsed as a number, boolean, etc, then it won't be initially
UNKNOWN, so that heuristic won't be applied.)

Now point 1 means that we don't really need to mess with keeping
state in the recursion context.  If we've determined at the level
of the OpExpr that we can do this, including checking that the
RHS operand IsA(Const), then we can just invoke deparseConst() on
it directly instead of recursing via deparseExpr().

Meanwhile, I suspect that point 2 might be best checked within
deparseConst() itself, as that contains both the decision and the
mechanism about how the Const will be printed.  So that suggests
that we should invent a new showtype code telling deparseConst()
to act this way, and then supply that code directly when we
invoke deparseConst directly from deparseOpExpr.

BTW, don't we also want to be able to optimize cases where the Const
is on the LHS rather than the RHS?

			regards, tom lane

  11. Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)

    Dian Fay <dian.m.fay@gmail.com> — 2021-10-24T05:10:52Z

    On Sun Sep 5, 2021 at 6:43 PM EDT, Tom Lane wrote:
> "Dian M Fay" <dian.m.fay@gmail.com> writes:
> > [ 0001-Suppress-explicit-casts-of-text-constants-in-postgre.patch ]
>
> I took a quick look at this. The restriction to type text seems like
> very obviously a hack rather than something we actually want; wouldn't
> it mean we fail to act in a large fraction of the cases where we'd
> like to suppress the cast?
>
> A second problem is that I don't think the business with a
> const_showtype
> context field is safe at all. As you've implemented it here, it would
> affect the entire RHS tree, including constants far down inside complex
> expressions that have nothing to do with the top-level semantics.
> (I didn't look closely, but I wonder if the regression failure you
> mentioned is associated with that.)
>
> I think that we only want to suppress the cast in cases where
> (1) the constant is directly an operand of the operator we're
> expecting the remote parser to use its same-type heuristic for, and
> (2) the constant will be deparsed as a string literal. (If it's
> deparsed as a number, boolean, etc, then it won't be initially
> UNKNOWN, so that heuristic won't be applied.)
>
> Now point 1 means that we don't really need to mess with keeping
> state in the recursion context. If we've determined at the level
> of the OpExpr that we can do this, including checking that the
> RHS operand IsA(Const), then we can just invoke deparseConst() on
> it directly instead of recursing via deparseExpr().
>
> Meanwhile, I suspect that point 2 might be best checked within
> deparseConst() itself, as that contains both the decision and the
> mechanism about how the Const will be printed. So that suggests
> that we should invent a new showtype code telling deparseConst()
> to act this way, and then supply that code directly when we
> invoke deparseConst directly from deparseOpExpr.
>
> BTW, don't we also want to be able to optimize cases where the Const
> is on the LHS rather than the RHS?
>
> regards, tom lane

Thanks Tom, that makes way more sense! I've attached a new patch which
tests operands and makes sure one side is a Const before feeding it to
deparseConst with a new showtype code, -2. The one regression is gone,
but I've left a couple of test output discrepancies for now which
showcase lost casts on the following predicates:

* date(c5) = '1970-01-17'::date
* ctid = '(0,2)'::tid

These aren't exactly failures -- both implicit string comparisons work
just fine -- but I don't know Postgres well enough to be sure that
that's true more generally. I did try checking that the non-Const member
of the predicate is a Var; that left the date cast alone, since date(c5)
is a FuncExpr, but obviously can't do anything about the tid.

There's also an interesting case where `val::text LIKE 'foo'` works when
val is an enum column in the local table, and breaks, castless, with an
operator mismatch when it's altered to text: Postgres' statement parser
recognizes the cast as redundant and creates a Var node instead of a
RelabelType (as it will for, say, `val::varchar(10)`) before the FDW is
even in the picture. It's a little discomfiting, but I suppose a certain
level of "caveat emptor" entails when disregarding foreign types.

> (val as enum on local and remote)
> explain verbose select * from test where (val::text) like 'foo';
> 
>  Foreign Scan on public.test  (cost=100.00..169.06 rows=8 width=28)
>    Output: id, val, on_day, ts, ts2
>    Filter: ((test.val)::text ~~ 'foo'::text)
>    Remote SQL: SELECT id, val, on_day, ts, ts2 FROM public.test
>
> (val as local text, remote enum)
> explain verbose select * from test where (val::text) like 'foo';
> 
>  Foreign Scan on public.test  (cost=100.00..122.90 rows=5 width=56)
>    Output: id, val, on_day, ts, ts2
>    Remote SQL: SELECT id, val, on_day, ts, ts2 FROM public.test WHERE ((val ~~ 'foo'))
>
> explain verbose select * from test where (val::varchar(10)) like 'foo';
>
>  Foreign Scan on public.test  (cost=100.00..125.46 rows=5 width=56)
>    Output: id, val, on_day, ts, ts2
>    Remote SQL: SELECT id, val, on_day, ts, ts2 FROM public.test WHERE ((val::character varying(10) ~~ 'foo'))

Outside that, deparseConst also contains a note about keeping the code
in sync with the parser (make_const in particular); from what I could
tell, I don't think there's anything in this that necessitates changes
there.

  12. Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-02T22:39:00Z

    "Dian M Fay" <dian.m.fay@gmail.com> writes:
> Thanks Tom, that makes way more sense! I've attached a new patch which
> tests operands and makes sure one side is a Const before feeding it to
> deparseConst with a new showtype code, -2. The one regression is gone,
> but I've left a couple of test output discrepancies for now which
> showcase lost casts on the following predicates:

> * date(c5) = '1970-01-17'::date
> * ctid = '(0,2)'::tid

> These aren't exactly failures -- both implicit string comparisons work
> just fine -- but I don't know Postgres well enough to be sure that
> that's true more generally.

These seem fine to me.  The parser heuristic that we're relying on
acts at the level of the operator --- it doesn't really care whether
the other input argument is a simple Var or not.

Note that we're *not* doing an "implicit string comparison" in either
case.  The point here is that the remote parser will resolve the
unknown-type literal as being the same type as the other operator input,
that is date or tid in these two cases.

That being the goal, I think you don't have the logic right at all,
even if it happens to accidentally work in the tested cases.  We
can only drop the cast if it's a binary operator and the two inputs
are of the same type.  Testing "leftType == form->oprleft" is pretty
close to a no-op, because the input will have been coerced to the
operator's expected type.  And the code as you had it could do
indefensible things with a unary operator.  (It's probably hard to
get here with a unary operator applied to a constant, but I'm not
sure it's impossible.)

Attached is a rewrite that does what I think we want to do, and
also adds comments because there weren't any.

Now that I've looked this over I'm starting to feel uncomfortable
again, because we can't actually be quite sure about how the remote
parser's heuristic will act.  What we're checking is that leftType
and rightType match, but that condition is applied to the inputs
*after implicit type coercion to the operator's input types*.
We can't be entirely sure about what our parser saw to begin with.
Perhaps it'd be a good idea to strip any implicit coercions on
the non-Const input before checking its type.  I'm not sure how
much that helps though.  For one thing, by the time this code
sees the expression, eval_const_expressions could have collapsed
coercion steps in a way that obscures how it looked originally.
For another thing, in the cases we're interested in, it's kind of
a stretch to suppose that implicit coercions applied locally are
a good model of the way things will look to the remote parser.

So I'm feeling a bit itchy.  I'm still willing to push forward
with this, but I won't be terribly surprised if it breaks cases
that ought to work and we end up having to revert it.

			regards, tom lane

  13. Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-02T23:10:25Z

    I wrote:
> Now that I've looked this over I'm starting to feel uncomfortable
> again, because we can't actually be quite sure about how the remote
> parser's heuristic will act.

Actually ... we could make that a lot safer by insisting that the
other input be a plain Var, which'd necessarily be a column of the
foreign table.  That would still cover most cases of practical
interest, I think, and it would remove any question of whether
implicit coercions had snuck in.  It's more restrictive than I'd
really like, but I think it's less likely to cause problems.

			regards, tom lane

  14. Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)

    Dian Fay <dian.m.fay@gmail.com> — 2021-11-07T23:31:39Z

    On Tue Nov 2, 2021 at 7:10 PM EDT, Tom Lane wrote:
> I wrote:
> > Now that I've looked this over I'm starting to feel uncomfortable
> > again, because we can't actually be quite sure about how the remote
> > parser's heuristic will act.
>
> Actually ... we could make that a lot safer by insisting that the
> other input be a plain Var, which'd necessarily be a column of the
> foreign table. That would still cover most cases of practical
> interest, I think, and it would remove any question of whether
> implicit coercions had snuck in. It's more restrictive than I'd
> really like, but I think it's less likely to cause problems.

Here's v6! I started with restricting cast suppression to Const-Var
comparisons as you suggested. A few tests did regress (relative to the
unrestricted version) right out of the gate with comparisons to varchar
columns, since those become RelabelType nodes instead of Vars. After
reading the notes on RelabelType in primnodes.h, I *think* that that
"dummy" coercion is distinct from the operator input type coercion
you're talking about here:

> What we're checking is that leftType and rightType match, but that
> condition is applied to the inputs *after implicit type coercion to
> the operator's input types*. We can't be entirely sure about what our
> parser saw to begin with. Perhaps it'd be a good idea to strip any
> implicit coercions on the non-Const input before checking its type.

I allowed RelabelTypes over Vars to suppress casts as well. It's working
for me so far and the varchar comparison tests are back to passing, sans
casts.

  15. Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-08T21:50:25Z

    "Dian M Fay" <dian.m.fay@gmail.com> writes:
> On Tue Nov 2, 2021 at 7:10 PM EDT, Tom Lane wrote:
>> Actually ... we could make that a lot safer by insisting that the
>> other input be a plain Var, which'd necessarily be a column of the
>> foreign table. That would still cover most cases of practical
>> interest, I think, and it would remove any question of whether
>> implicit coercions had snuck in. It's more restrictive than I'd
>> really like, but I think it's less likely to cause problems.

> I allowed RelabelTypes over Vars to suppress casts as well. It's working
> for me so far and the varchar comparison tests are back to passing, sans
> casts.

Um.  I doubt that that's any safer than the v5 patch.  As an example,
casting between int4 and oid is just a RelabelType, but the comparison
semantics change completely (signed vs. unsigned); so there's not a
good reason to think this is constraining things more than v5 did.

It might be better if you'd further restricted the structure to be only
COERCE_IMPLICIT_CAST RelabelTypes, since we don't normally make casts
implicit if they significantly change semantics.  Also, this'd ensure
that the operand printed for the remote server is just a bare Var
(cf. deparseRelabelType).  But even with that I'm feeling antsy about
whether this will allow any semantic surprises.

			regards, tom lane

  16. Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)

    Dian Fay <dian.m.fay@gmail.com> — 2021-11-11T04:58:44Z

    On Mon Nov 8, 2021 at 4:50 PM EST, Tom Lane wrote:
> Um. I doubt that that's any safer than the v5 patch. As an example,
> casting between int4 and oid is just a RelabelType, but the comparison
> semantics change completely (signed vs. unsigned); so there's not a
> good reason to think this is constraining things more than v5 did.
>
> It might be better if you'd further restricted the structure to be only
> COERCE_IMPLICIT_CAST RelabelTypes, since we don't normally make casts
> implicit if they significantly change semantics. Also, this'd ensure
> that the operand printed for the remote server is just a bare Var
> (cf. deparseRelabelType). But even with that I'm feeling antsy about
> whether this will allow any semantic surprises.

I've split the suppression for RelabelTypes with implicit cast check
into a second patch over the core v7 change. As far as testing goes, \dC
lists implicit casts, but most of those I've tried seem to wind up
deparsing as Vars. I've been able to manifest RelabelTypes with varchar,
cidr, and remote char to local varchar, but that's about it. Any ideas
for validating it further, off the top of your head?

  17. Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-11T20:36:53Z

    "Dian M Fay" <dian.m.fay@gmail.com> writes:
> I've split the suppression for RelabelTypes with implicit cast check
> into a second patch over the core v7 change. As far as testing goes, \dC
> lists implicit casts, but most of those I've tried seem to wind up
> deparsing as Vars. I've been able to manifest RelabelTypes with varchar,
> cidr, and remote char to local varchar, but that's about it. Any ideas
> for validating it further, off the top of your head?

I thought about this some more and realized exactly why I wanted to
restrict the change to cases where the other side is a plain foreign Var:
that way, if anything surprising happens, we can blame it directly on the
user having declared a local column with a different type from the
remote column.

That being the case, I took a closer look at deparseVar and realized that
we can't simply check "IsA(node, Var)": some Vars in the expression can
belong to local tables.  We need to verify that the Var is one that will
print as a remote column reference.

So that leads me to v8, attached.  I think we are getting there.

			regards, tom lane

  18. Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)

    Dian Fay <dian.m.fay@gmail.com> — 2021-11-12T01:07:14Z

    On Thu Nov 11, 2021 at 3:36 PM EST, Tom Lane wrote:
> I thought about this some more and realized exactly why I wanted to
> restrict the change to cases where the other side is a plain foreign
> Var: that way, if anything surprising happens, we can blame it
> directly on the user having declared a local column with a different
> type from the remote column.
>
> That being the case, I took a closer look at deparseVar and realized
> that we can't simply check "IsA(node, Var)": some Vars in the
> expression can belong to local tables. We need to verify that the Var
> is one that will print as a remote column reference.

Eminently reasonable all around! `git apply` insisted that the v8 patch
didn't (apply, that is), but `patch -p1` liked it fine. I've put it
through a few paces and it seems good; what needs to happen next?

  19. Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-12T16:54:20Z

    "Dian M Fay" <dian.m.fay@gmail.com> writes:
> Eminently reasonable all around! `git apply` insisted that the v8 patch
> didn't (apply, that is), but `patch -p1` liked it fine. I've put it
> through a few paces and it seems good; what needs to happen next?

I don't see anything else to do except shove it out into the light
of day and see what happens.  Hence, pushed.

As I remarked in the commit message:

>> One point that I (tgl) remain slightly uncomfortable with is that we
>> will ignore an implicit RelabelType when deciding if the non-Const input
>> is a plain Var.  That makes it a little squishy to argue that the remote
>> should resolve the Const as being of the same type as its Var, because
>> then our Const is not the same type as our Var.  However, if we don't do
>> that, then this hack won't work as desired if the user chooses to use
>> varchar rather than text to represent some remote column.  That seems
>> useful, so do it like this for now.  We might have to give up the
>> RelabelType-ignoring bit if any problems surface.

I think we can await complaints before doing more, but I wanted that
bit on record for anyone perusing the archives.

			regards, tom lane