Skip to content

Commit f8abb0f

Browse files
tglsfdctglsfdc
committed
postgres_fdw: suppress casts on constants in limited cases.
When deparsing an expression of the form "remote_var OP constant", we'd normally apply a cast to the constant to make sure that the remote parser thinks it's of the same type we do. However, doing so is often not necessary, and it causes problems if the user has intentionally declared the local column as being of a different type than the remote column. A plausible use-case for that is using text to represent a type that's an enum on the remote side. A comparison on such a column will get shipped as "var = 'foo'::text", which blows up on the remote side because there's no enum = text operator. But if we simply leave off the explicit cast, the comparison will do exactly what the user wants. It's possible to do this without major risk of semantic problems, by relying on the longstanding parser heuristic that "if one operand of an operator is of type unknown, while the other one has a known type, assume that the unknown operand is also of that type". Hence, this patch leaves off the cast only if (a) the operator inputs have the same type locally; (b) the constant will print as a string literal or NULL, both of which are initially taken as type unknown; and (c) the non-Const input is a plain foreign Var. Rule (c) guarantees that the remote parser will know the type of the non-Const input; moreover, it means that if this cast-omission does cause any semantic surprises, that can only happen in cases where the local column has a different type than the remote column. That wasn't guaranteed to work anyway, and this patch should represent a net usability gain for such cases. 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. Dian Fay, with review and kibitzing by me Discussion: https://postgr.es/m/C9LU294V7K4F.34LRRDU449O45@lamia
1 parent 1593998 commit f8abb0f

File tree

3 files changed

+212
-47
lines changed

3 files changed

+212
-47
lines changed

‎contrib/postgres_fdw/deparse.c

+108-8
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,7 @@ static void deparseParam(Param *node, deparse_expr_cxt *context);
152152
static void deparseSubscriptingRef(SubscriptingRef *node, deparse_expr_cxt *context);
153153
static void deparseFuncExpr(FuncExpr *node, deparse_expr_cxt *context);
154154
static void deparseOpExpr(OpExpr *node, deparse_expr_cxt *context);
155+
static bool isPlainForeignVar(Expr *node, deparse_expr_cxt *context);
155156
static void deparseOperatorName(StringInfo buf, Form_pg_operator opform);
156157
static void deparseDistinctExpr(DistinctExpr *node, deparse_expr_cxt *context);
157158
static void deparseScalarArrayOpExpr(ScalarArrayOpExpr *node,
@@ -2695,9 +2696,14 @@ deparseVar(Var *node, deparse_expr_cxt *context)
26952696
* Deparse given constant value into context->buf.
26962697
*
26972698
* This function has to be kept in sync with ruleutils.c's get_const_expr.
2698-
* As for that function, showtype can be -1 to never show "::typename" decoration,
2699-
* or +1 to always show it, or 0 to show it only if the constant wouldn't be assumed
2700-
* to be the right type by default.
2699+
*
2700+
* As in that function, showtype can be -1 to never show "::typename"
2701+
* decoration, +1 to always show it, or 0 to show it only if the constant
2702+
* wouldn't be assumed to be the right type by default.
2703+
*
2704+
* In addition, this code allows showtype to be -2 to indicate that we should
2705+
* not show "::typename" decoration if the constant is printed as an untyped
2706+
* literal or NULL (while in other cases, behaving as for showtype == 0).
27012707
*/
27022708
static void
27032709
deparseConst(Const *node, deparse_expr_cxt *context, int showtype)
@@ -2707,6 +2713,7 @@ deparseConst(Const *node, deparse_expr_cxt *context, int showtype)
27072713
bool typIsVarlena;
27082714
char *extval;
27092715
bool isfloat = false;
2716+
bool isstring = false;
27102717
bool needlabel;
27112718

27122719
if (node->constisnull)
@@ -2762,13 +2769,14 @@ deparseConst(Const *node, deparse_expr_cxt *context, int showtype)
27622769
break;
27632770
default:
27642771
deparseStringLiteral(buf, extval);
2772+
isstring = true;
27652773
break;
27662774
}
27672775

27682776
pfree(extval);
27692777

2770-
if (showtype < 0)
2771-
return;
2778+
if (showtype == -1)
2779+
return; /* never print type label */
27722780

27732781
/*
27742782
* For showtype == 0, append ::typename unless the constant will be
@@ -2788,7 +2796,13 @@ deparseConst(Const *node, deparse_expr_cxt *context, int showtype)
27882796
needlabel = !isfloat || (node->consttypmod >= 0);
27892797
break;
27902798
default:
2791-
needlabel = true;
2799+
if (showtype == -2)
2800+
{
2801+
/* label unless we printed it as an untyped string */
2802+
needlabel = !isstring;
2803+
}
2804+
else
2805+
needlabel = true;
27922806
break;
27932807
}
27942808
if (needlabel || showtype > 0)
@@ -2953,6 +2967,8 @@ deparseOpExpr(OpExpr *node, deparse_expr_cxt *context)
29532967
StringInfo buf = context->buf;
29542968
HeapTuple tuple;
29552969
Form_pg_operator form;
2970+
Expr *right;
2971+
bool canSuppressRightConstCast = false;
29562972
char oprkind;
29572973

29582974
/* Retrieve information about the operator from system catalog. */
@@ -2966,13 +2982,58 @@ deparseOpExpr(OpExpr *node, deparse_expr_cxt *context)
29662982
Assert((oprkind == 'l' && list_length(node->args) == 1) ||
29672983
(oprkind == 'b' && list_length(node->args) == 2));
29682984

2985+
right = llast(node->args);
2986+
29692987
/* Always parenthesize the expression. */
29702988
appendStringInfoChar(buf, '(');
29712989

29722990
/* Deparse left operand, if any. */
29732991
if (oprkind == 'b')
29742992
{
2975-
deparseExpr(linitial(node->args), context);
2993+
Expr *left = linitial(node->args);
2994+
Oid leftType = exprType((Node *) left);
2995+
Oid rightType = exprType((Node *) right);
2996+
bool canSuppressLeftConstCast = false;
2997+
2998+
/*
2999+
* When considering a binary operator, if one operand is a Const that
3000+
* can be printed as a bare string literal or NULL (i.e., it will look
3001+
* like type UNKNOWN to the remote parser), the Const normally
3002+
* receives an explicit cast to the operator's input type. However,
3003+
* in Const-to-Var comparisons where both operands are of the same
3004+
* type, we prefer to suppress the explicit cast, leaving the Const's
3005+
* type resolution up to the remote parser. The remote's resolution
3006+
* heuristic will assume that an unknown input type being compared to
3007+
* a known input type is of that known type as well.
3008+
*
3009+
* This hack allows some cases to succeed where a remote column is
3010+
* declared with a different type in the local (foreign) table. By
3011+
* emitting "foreigncol = 'foo'" not "foreigncol = 'foo'::text" or the
3012+
* like, we allow the remote parser to pick an "=" operator that's
3013+
* compatible with whatever type the remote column really is, such as
3014+
* an enum.
3015+
*
3016+
* We allow cast suppression to happen only when the other operand is
3017+
* a plain foreign Var. Although the remote's unknown-type heuristic
3018+
* would apply to other cases just as well, we would be taking a
3019+
* bigger risk that the inferred type is something unexpected. With
3020+
* this restriction, if anything goes wrong it's the user's fault for
3021+
* not declaring the local column with the same type as the remote
3022+
* column.
3023+
*/
3024+
if (leftType == rightType)
3025+
{
3026+
if (IsA(left, Const))
3027+
canSuppressLeftConstCast = isPlainForeignVar(right, context);
3028+
else if (IsA(right, Const))
3029+
canSuppressRightConstCast = isPlainForeignVar(left, context);
3030+
}
3031+
3032+
if (canSuppressLeftConstCast)
3033+
deparseConst((Const *) left, context, -2);
3034+
else
3035+
deparseExpr(left, context);
3036+
29763037
appendStringInfoChar(buf, ' ');
29773038
}
29783039

@@ -2981,13 +3042,52 @@ deparseOpExpr(OpExpr *node, deparse_expr_cxt *context)
29813042

29823043
/* Deparse right operand. */
29833044
appendStringInfoChar(buf, ' ');
2984-
deparseExpr(llast(node->args), context);
3045+
3046+
if (canSuppressRightConstCast)
3047+
deparseConst((Const *) right, context, -2);
3048+
else
3049+
deparseExpr(right, context);
29853050

29863051
appendStringInfoChar(buf, ')');
29873052

29883053
ReleaseSysCache(tuple);
29893054
}
29903055

3056+
/*
3057+
* Will "node" deparse as a plain foreign Var?
3058+
*/
3059+
static bool
3060+
isPlainForeignVar(Expr *node, deparse_expr_cxt *context)
3061+
{
3062+
/*
3063+
* We allow the foreign Var to have an implicit RelabelType, mainly so
3064+
* that this'll work with varchar columns. Note that deparseRelabelType
3065+
* will not print such a cast, so we're not breaking the restriction that
3066+
* the expression print as a plain Var. We won't risk it for an implicit
3067+
* cast that requires a function, nor for non-implicit RelabelType; such
3068+
* cases seem too likely to involve semantics changes compared to what
3069+
* would happen on the remote side.
3070+
*/
3071+
if (IsA(node, RelabelType) &&
3072+
((RelabelType *) node)->relabelformat == COERCE_IMPLICIT_CAST)
3073+
node = ((RelabelType *) node)->arg;
3074+
3075+
if (IsA(node, Var))
3076+
{
3077+
/*
3078+
* The Var must be one that'll deparse as a foreign column reference
3079+
* (cf. deparseVar).
3080+
*/
3081+
Var *var = (Var *) node;
3082+
Relids relids = context->scanrel->relids;
3083+
3084+
if (bms_is_member(var->varno, relids) && var->varlevelsup == 0)
3085+
return true;
3086+
}
3087+
3088+
return false;
3089+
}
3090+
29913091
/*
29923092
* Print the name of an operator.
29933093
*/

0 commit comments

Comments
 (0)