Skip to content

Commit b7dcb2d

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 fee2275 commit b7dcb2d

File tree

3 files changed

+135
-66
lines changed

3 files changed

+135
-66
lines changed

‎contrib/postgres_fdw/deparse.c

Lines changed: 57 additions & 32 deletions
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-2013, 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
@@ -271,13 +275,24 @@ foreign_expr_walker(Node *node,
271275
else
272276
{
273277
/* Var belongs to some other table */
274-
if (var->varcollid != InvalidOid &&
275-
var->varcollid != DEFAULT_COLLATION_OID)
276-
return false;
277-
278-
/* We can consider that it doesn't set collation */
279-
collation = InvalidOid;
280-
state = FDW_COLLATE_NONE;
278+
collation = var->varcollid;
279+
if (collation == InvalidOid ||
280+
collation == DEFAULT_COLLATION_OID)
281+
{
282+
/*
283+
* It's noncollatable, or it's safe to combine with a
284+
* collatable foreign Var, so set state to NONE.
285+
*/
286+
state = FDW_COLLATE_NONE;
287+
}
288+
else
289+
{
290+
/*
291+
* Do not fail right away, since the Var might appear
292+
* in a collation-insensitive context.
293+
*/
294+
state = FDW_COLLATE_UNSAFE;
295+
}
281296
}
282297
}
283298
break;
@@ -287,31 +302,31 @@ foreign_expr_walker(Node *node,
287302

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

306321
/*
307-
* Collation handling is same as for Consts.
322+
* Collation rule is same as for Consts and non-foreign Vars.
308323
*/
309-
if (p->paramcollid != InvalidOid &&
310-
p->paramcollid != DEFAULT_COLLATION_OID)
311-
return false;
312-
313-
collation = InvalidOid;
314-
state = FDW_COLLATE_NONE;
324+
collation = p->paramcollid;
325+
if (collation == InvalidOid ||
326+
collation == DEFAULT_COLLATION_OID)
327+
state = FDW_COLLATE_NONE;
328+
else
329+
state = FDW_COLLATE_UNSAFE;
315330
}
316331
break;
317332
case T_ArrayRef:
@@ -347,6 +362,8 @@ foreign_expr_walker(Node *node,
347362
else if (inner_cxt.state == FDW_COLLATE_SAFE &&
348363
collation == inner_cxt.collation)
349364
state = FDW_COLLATE_SAFE;
365+
else if (collation == DEFAULT_COLLATION_OID)
366+
state = FDW_COLLATE_NONE;
350367
else
351368
state = FDW_COLLATE_UNSAFE;
352369
}
@@ -392,6 +409,8 @@ foreign_expr_walker(Node *node,
392409
else if (inner_cxt.state == FDW_COLLATE_SAFE &&
393410
collation == inner_cxt.collation)
394411
state = FDW_COLLATE_SAFE;
412+
else if (collation == DEFAULT_COLLATION_OID)
413+
state = FDW_COLLATE_NONE;
395414
else
396415
state = FDW_COLLATE_UNSAFE;
397416
}
@@ -433,6 +452,8 @@ foreign_expr_walker(Node *node,
433452
else if (inner_cxt.state == FDW_COLLATE_SAFE &&
434453
collation == inner_cxt.collation)
435454
state = FDW_COLLATE_SAFE;
455+
else if (collation == DEFAULT_COLLATION_OID)
456+
state = FDW_COLLATE_NONE;
436457
else
437458
state = FDW_COLLATE_UNSAFE;
438459
}
@@ -482,14 +503,16 @@ foreign_expr_walker(Node *node,
482503

483504
/*
484505
* RelabelType must not introduce a collation not derived from
485-
* an input foreign Var.
506+
* an input foreign Var (same logic as for a real function).
486507
*/
487508
collation = r->resultcollid;
488509
if (collation == InvalidOid)
489510
state = FDW_COLLATE_NONE;
490511
else if (inner_cxt.state == FDW_COLLATE_SAFE &&
491512
collation == inner_cxt.collation)
492513
state = FDW_COLLATE_SAFE;
514+
else if (collation == DEFAULT_COLLATION_OID)
515+
state = FDW_COLLATE_NONE;
493516
else
494517
state = FDW_COLLATE_UNSAFE;
495518
}
@@ -539,14 +562,16 @@ foreign_expr_walker(Node *node,
539562

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

‎contrib/postgres_fdw/expected/postgres_fdw.out

Lines changed: 70 additions & 31 deletions
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

Lines changed: 8 additions & 3 deletions
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)