Skip to content

Commit 0da864c

Browse files
tglsfdctglsfdc
committed
Improve handling of collations in contrib/postgres_fdw.
If we have a local Var of say varchar type with default collation, and we apply a RelabelType to convert that to text with default collation, we don't want to consider that as creating an FDW_COLLATE_UNSAFE situation. It should be okay to compare that to a remote Var, so long as the remote Var determines the comparison collation. (When we actually ship such an expression to the remote side, the local Var would become a Param with default collation, meaning the remote Var would in fact control the comparison collation, because non-default implicit collation overrides default implicit collation in parse_collate.c.) To fix, be more precise about what FDW_COLLATE_NONE means: it applies either to a noncollatable data type or to a collatable type with default collation, if that collation can't be traced to a remote Var. (When it can, FDW_COLLATE_SAFE is appropriate.) We were essentially using that interpretation already at the Var/Const/Param level, but we weren't bubbling it up properly. An alternative fix would be to introduce a separate FDW_COLLATE_DEFAULT value to describe the second situation, but that would add more code without changing the actual behavior, so it didn't seem worthwhile. Also, since we're clarifying the rule to be that we care about whether operator/function input collations match, there seems no need to fail immediately upon seeing a Const/Param/non-foreign-Var with nondefault collation. We only have to reject if it appears in a collation-sensitive context (for example, "var IS NOT NULL" is perfectly safe from a collation standpoint, whatever collation the var has). So just set the state to UNSAFE rather than failing immediately. Per report from Jeevan Chalke. This essentially corrects some sloppy thinking in commit ed3ddf9, so back-patch to 9.3 where that logic appeared.
1 parent 4ff753c commit 0da864c

File tree

3 files changed

+135
-66
lines changed

3 files changed

+135
-66
lines changed

‎contrib/postgres_fdw/deparse.c

+57-32
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,12 @@
1717
* We do not consider that it is ever safe to send COLLATE expressions to
1818
* the remote server: it might not have the same collation names we do.
1919
* (Later we might consider it safe to send COLLATE "C", but even that would
20-
* fail on old remote servers.) An expression is considered safe to send only
21-
* if all collations used in it are traceable to Var(s) of the foreign table.
22-
* That implies that if the remote server gets a different answer than we do,
23-
* the foreign table's columns are not marked with collations that match the
24-
* remote table's columns, which we can consider to be user error.
20+
* fail on old remote servers.) An expression is considered safe to send
21+
* only if all operator/function input collations used in it are traceable to
22+
* Var(s) of the foreign table. That implies that if the remote server gets
23+
* a different answer than we do, the foreign table's columns are not marked
24+
* with collations that match the remote table's columns, which we can
25+
* consider to be user error.
2526
*
2627
* Portions Copyright (c) 2012-2014, PostgreSQL Global Development Group
2728
*
@@ -68,9 +69,12 @@ typedef struct foreign_glob_cxt
6869
*/
6970
typedef enum
7071
{
71-
FDW_COLLATE_NONE, /* expression is of a noncollatable type */
72+
FDW_COLLATE_NONE, /* expression is of a noncollatable type, or
73+
* it has default collation that is not
74+
* traceable to a foreign Var */
7275
FDW_COLLATE_SAFE, /* collation derives from a foreign Var */
73-
FDW_COLLATE_UNSAFE /* collation derives from something else */
76+
FDW_COLLATE_UNSAFE /* collation is non-default and derives from
77+
* something other than a foreign Var */
7478
} FDWCollateState;
7579

7680
typedef struct foreign_loc_cxt
@@ -272,13 +276,24 @@ foreign_expr_walker(Node *node,
272276
else
273277
{
274278
/* Var belongs to some other table */
275-
if (var->varcollid != InvalidOid &&
276-
var->varcollid != DEFAULT_COLLATION_OID)
277-
return false;
278-
279-
/* We can consider that it doesn't set collation */
280-
collation = InvalidOid;
281-
state = FDW_COLLATE_NONE;
279+
collation = var->varcollid;
280+
if (collation == InvalidOid ||
281+
collation == DEFAULT_COLLATION_OID)
282+
{
283+
/*
284+
* It's noncollatable, or it's safe to combine with a
285+
* collatable foreign Var, so set state to NONE.
286+
*/
287+
state = FDW_COLLATE_NONE;
288+
}
289+
else
290+
{
291+
/*
292+
* Do not fail right away, since the Var might appear
293+
* in a collation-insensitive context.
294+
*/
295+
state = FDW_COLLATE_UNSAFE;
296+
}
282297
}
283298
}
284299
break;
@@ -288,31 +303,31 @@ foreign_expr_walker(Node *node,
288303

289304
/*
290305
* If the constant has nondefault collation, either it's of a
291-
* non-builtin type, or it reflects folding of a CollateExpr;
292-
* either way, it's unsafe to send to the remote.
306+
* non-builtin type, or it reflects folding of a CollateExpr.
307+
* It's unsafe to send to the remote unless it's used in a
308+
* non-collation-sensitive context.
293309
*/
294-
if (c->constcollid != InvalidOid &&
295-
c->constcollid != DEFAULT_COLLATION_OID)
296-
return false;
297-
298-
/* Otherwise, we can consider that it doesn't set collation */
299-
collation = InvalidOid;
300-
state = FDW_COLLATE_NONE;
310+
collation = c->constcollid;
311+
if (collation == InvalidOid ||
312+
collation == DEFAULT_COLLATION_OID)
313+
state = FDW_COLLATE_NONE;
314+
else
315+
state = FDW_COLLATE_UNSAFE;
301316
}
302317
break;
303318
case T_Param:
304319
{
305320
Param *p = (Param *) node;
306321

307322
/*
308-
* Collation handling is same as for Consts.
323+
* Collation rule is same as for Consts and non-foreign Vars.
309324
*/
310-
if (p->paramcollid != InvalidOid &&
311-
p->paramcollid != DEFAULT_COLLATION_OID)
312-
return false;
313-
314-
collation = InvalidOid;
315-
state = FDW_COLLATE_NONE;
325+
collation = p->paramcollid;
326+
if (collation == InvalidOid ||
327+
collation == DEFAULT_COLLATION_OID)
328+
state = FDW_COLLATE_NONE;
329+
else
330+
state = FDW_COLLATE_UNSAFE;
316331
}
317332
break;
318333
case T_ArrayRef:
@@ -348,6 +363,8 @@ foreign_expr_walker(Node *node,
348363
else if (inner_cxt.state == FDW_COLLATE_SAFE &&
349364
collation == inner_cxt.collation)
350365
state = FDW_COLLATE_SAFE;
366+
else if (collation == DEFAULT_COLLATION_OID)
367+
state = FDW_COLLATE_NONE;
351368
else
352369
state = FDW_COLLATE_UNSAFE;
353370
}
@@ -393,6 +410,8 @@ foreign_expr_walker(Node *node,
393410
else if (inner_cxt.state == FDW_COLLATE_SAFE &&
394411
collation == inner_cxt.collation)
395412
state = FDW_COLLATE_SAFE;
413+
else if (collation == DEFAULT_COLLATION_OID)
414+
state = FDW_COLLATE_NONE;
396415
else
397416
state = FDW_COLLATE_UNSAFE;
398417
}
@@ -434,6 +453,8 @@ foreign_expr_walker(Node *node,
434453
else if (inner_cxt.state == FDW_COLLATE_SAFE &&
435454
collation == inner_cxt.collation)
436455
state = FDW_COLLATE_SAFE;
456+
else if (collation == DEFAULT_COLLATION_OID)
457+
state = FDW_COLLATE_NONE;
437458
else
438459
state = FDW_COLLATE_UNSAFE;
439460
}
@@ -483,14 +504,16 @@ foreign_expr_walker(Node *node,
483504

484505
/*
485506
* RelabelType must not introduce a collation not derived from
486-
* an input foreign Var.
507+
* an input foreign Var (same logic as for a real function).
487508
*/
488509
collation = r->resultcollid;
489510
if (collation == InvalidOid)
490511
state = FDW_COLLATE_NONE;
491512
else if (inner_cxt.state == FDW_COLLATE_SAFE &&
492513
collation == inner_cxt.collation)
493514
state = FDW_COLLATE_SAFE;
515+
else if (collation == DEFAULT_COLLATION_OID)
516+
state = FDW_COLLATE_NONE;
494517
else
495518
state = FDW_COLLATE_UNSAFE;
496519
}
@@ -540,14 +563,16 @@ foreign_expr_walker(Node *node,
540563

541564
/*
542565
* ArrayExpr must not introduce a collation not derived from
543-
* an input foreign Var.
566+
* an input foreign Var (same logic as for a function).
544567
*/
545568
collation = a->array_collid;
546569
if (collation == InvalidOid)
547570
state = FDW_COLLATE_NONE;
548571
else if (inner_cxt.state == FDW_COLLATE_SAFE &&
549572
collation == inner_cxt.collation)
550573
state = FDW_COLLATE_SAFE;
574+
else if (collation == DEFAULT_COLLATION_OID)
575+
state = FDW_COLLATE_NONE;
551576
else
552577
state = FDW_COLLATE_UNSAFE;
553578
}

‎contrib/postgres_fdw/expected/postgres_fdw.out

+70-31
Original file line numberDiff line numberDiff line change
@@ -999,71 +999,110 @@ COMMIT;
999999
-- ===================================================================
10001000
-- test handling of collations
10011001
-- ===================================================================
1002-
create table loct3 (f1 text collate "C", f2 text);
1003-
create foreign table ft3 (f1 text collate "C", f2 text)
1004-
server loopback options (table_name 'loct3');
1002+
create table loct3 (f1 text collate "C" unique, f2 text, f3 varchar(10) unique);
1003+
create foreign table ft3 (f1 text collate "C", f2 text, f3 varchar(10))
1004+
server loopback options (table_name 'loct3', use_remote_estimate 'true');
10051005
-- can be sent to remote
10061006
explain (verbose, costs off) select * from ft3 where f1 = 'foo';
1007-
QUERY PLAN
1008-
--------------------------------------------------------------------------
1007+
QUERY PLAN
1008+
------------------------------------------------------------------------------
10091009
Foreign Scan on public.ft3
1010-
Output: f1, f2
1011-
Remote SQL: SELECT f1, f2 FROM public.loct3 WHERE ((f1 = 'foo'::text))
1010+
Output: f1, f2, f3
1011+
Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f1 = 'foo'::text))
10121012
(3 rows)
10131013

10141014
explain (verbose, costs off) select * from ft3 where f1 COLLATE "C" = 'foo';
1015-
QUERY PLAN
1016-
--------------------------------------------------------------------------
1015+
QUERY PLAN
1016+
------------------------------------------------------------------------------
10171017
Foreign Scan on public.ft3
1018-
Output: f1, f2
1019-
Remote SQL: SELECT f1, f2 FROM public.loct3 WHERE ((f1 = 'foo'::text))
1018+
Output: f1, f2, f3
1019+
Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f1 = 'foo'::text))
10201020
(3 rows)
10211021

10221022
explain (verbose, costs off) select * from ft3 where f2 = 'foo';
1023-
QUERY PLAN
1024-
--------------------------------------------------------------------------
1023+
QUERY PLAN
1024+
------------------------------------------------------------------------------
10251025
Foreign Scan on public.ft3
1026-
Output: f1, f2
1027-
Remote SQL: SELECT f1, f2 FROM public.loct3 WHERE ((f2 = 'foo'::text))
1026+
Output: f1, f2, f3
1027+
Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f2 = 'foo'::text))
10281028
(3 rows)
10291029

1030+
explain (verbose, costs off) select * from ft3 where f3 = 'foo';
1031+
QUERY PLAN
1032+
------------------------------------------------------------------------------
1033+
Foreign Scan on public.ft3
1034+
Output: f1, f2, f3
1035+
Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f3 = 'foo'::text))
1036+
(3 rows)
1037+
1038+
explain (verbose, costs off) select * from ft3 f, loct3 l
1039+
where f.f3 = l.f3 and l.f1 = 'foo';
1040+
QUERY PLAN
1041+
--------------------------------------------------------------------------------------------------
1042+
Nested Loop
1043+
Output: f.f1, f.f2, f.f3, l.f1, l.f2, l.f3
1044+
-> Index Scan using loct3_f1_key on public.loct3 l
1045+
Output: l.f1, l.f2, l.f3
1046+
Index Cond: (l.f1 = 'foo'::text)
1047+
-> Foreign Scan on public.ft3 f
1048+
Output: f.f1, f.f2, f.f3
1049+
Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE (($1::character varying(10) = f3))
1050+
(8 rows)
1051+
10301052
-- can't be sent to remote
10311053
explain (verbose, costs off) select * from ft3 where f1 COLLATE "POSIX" = 'foo';
1032-
QUERY PLAN
1033-
-----------------------------------------------
1054+
QUERY PLAN
1055+
---------------------------------------------------
10341056
Foreign Scan on public.ft3
1035-
Output: f1, f2
1057+
Output: f1, f2, f3
10361058
Filter: ((ft3.f1)::text = 'foo'::text)
1037-
Remote SQL: SELECT f1, f2 FROM public.loct3
1059+
Remote SQL: SELECT f1, f2, f3 FROM public.loct3
10381060
(4 rows)
10391061

10401062
explain (verbose, costs off) select * from ft3 where f1 = 'foo' COLLATE "C";
1041-
QUERY PLAN
1042-
-----------------------------------------------
1063+
QUERY PLAN
1064+
---------------------------------------------------
10431065
Foreign Scan on public.ft3
1044-
Output: f1, f2
1066+
Output: f1, f2, f3
10451067
Filter: (ft3.f1 = 'foo'::text COLLATE "C")
1046-
Remote SQL: SELECT f1, f2 FROM public.loct3
1068+
Remote SQL: SELECT f1, f2, f3 FROM public.loct3
10471069
(4 rows)
10481070

10491071
explain (verbose, costs off) select * from ft3 where f2 COLLATE "C" = 'foo';
1050-
QUERY PLAN
1051-
-----------------------------------------------
1072+
QUERY PLAN
1073+
---------------------------------------------------
10521074
Foreign Scan on public.ft3
1053-
Output: f1, f2
1075+
Output: f1, f2, f3
10541076
Filter: ((ft3.f2)::text = 'foo'::text)
1055-
Remote SQL: SELECT f1, f2 FROM public.loct3
1077+
Remote SQL: SELECT f1, f2, f3 FROM public.loct3
10561078
(4 rows)
10571079

10581080
explain (verbose, costs off) select * from ft3 where f2 = 'foo' COLLATE "C";
1059-
QUERY PLAN
1060-
-----------------------------------------------
1081+
QUERY PLAN
1082+
---------------------------------------------------
10611083
Foreign Scan on public.ft3
1062-
Output: f1, f2
1084+
Output: f1, f2, f3
10631085
Filter: (ft3.f2 = 'foo'::text COLLATE "C")
1064-
Remote SQL: SELECT f1, f2 FROM public.loct3
1086+
Remote SQL: SELECT f1, f2, f3 FROM public.loct3
10651087
(4 rows)
10661088

1089+
explain (verbose, costs off) select * from ft3 f, loct3 l
1090+
where f.f3 = l.f3 COLLATE "POSIX" and l.f1 = 'foo';
1091+
QUERY PLAN
1092+
-------------------------------------------------------------
1093+
Hash Join
1094+
Output: f.f1, f.f2, f.f3, l.f1, l.f2, l.f3
1095+
Hash Cond: ((f.f3)::text = (l.f3)::text)
1096+
-> Foreign Scan on public.ft3 f
1097+
Output: f.f1, f.f2, f.f3
1098+
Remote SQL: SELECT f1, f2, f3 FROM public.loct3
1099+
-> Hash
1100+
Output: l.f1, l.f2, l.f3
1101+
-> Index Scan using loct3_f1_key on public.loct3 l
1102+
Output: l.f1, l.f2, l.f3
1103+
Index Cond: (l.f1 = 'foo'::text)
1104+
(11 rows)
1105+
10671106
-- ===================================================================
10681107
-- test writable foreign table stuff
10691108
-- ===================================================================

‎contrib/postgres_fdw/sql/postgres_fdw.sql

+8-3
Original file line numberDiff line numberDiff line change
@@ -310,19 +310,24 @@ COMMIT;
310310
-- ===================================================================
311311
-- test handling of collations
312312
-- ===================================================================
313-
create table loct3 (f1 text collate "C", f2 text);
314-
create foreign table ft3 (f1 text collate "C", f2 text)
315-
server loopback options (table_name 'loct3');
313+
create table loct3 (f1 text collate "C" unique, f2 text, f3 varchar(10) unique);
314+
create foreign table ft3 (f1 text collate "C", f2 text, f3 varchar(10))
315+
server loopback options (table_name 'loct3', use_remote_estimate 'true');
316316

317317
-- can be sent to remote
318318
explain (verbose, costs off) select * from ft3 where f1 = 'foo';
319319
explain (verbose, costs off) select * from ft3 where f1 COLLATE "C" = 'foo';
320320
explain (verbose, costs off) select * from ft3 where f2 = 'foo';
321+
explain (verbose, costs off) select * from ft3 where f3 = 'foo';
322+
explain (verbose, costs off) select * from ft3 f, loct3 l
323+
where f.f3 = l.f3 and l.f1 = 'foo';
321324
-- can't be sent to remote
322325
explain (verbose, costs off) select * from ft3 where f1 COLLATE "POSIX" = 'foo';
323326
explain (verbose, costs off) select * from ft3 where f1 = 'foo' COLLATE "C";
324327
explain (verbose, costs off) select * from ft3 where f2 COLLATE "C" = 'foo';
325328
explain (verbose, costs off) select * from ft3 where f2 = 'foo' COLLATE "C";
329+
explain (verbose, costs off) select * from ft3 f, loct3 l
330+
where f.f3 = l.f3 COLLATE "POSIX" and l.f1 = 'foo';
326331

327332
-- ===================================================================
328333
-- test writable foreign table stuff

0 commit comments

Comments
 (0)