Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types) - Mailing list pgsql-hackers

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.