Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit c402b02

Browse filesBrowse files
tglsfdctglsfdc
committed
Fix old bug with coercing the result of a COLLATE expression.
There are hacks in parse_coerce.c to push down a requested coercion to below any CollateExpr that may appear. However, we did that even if the requested data type is non-collatable, leading to an invalid expression tree in which CollateExpr is applied to a non-collatable type. The fix is just to drop the CollateExpr altogether, reasoning that it's useless. This bug is ten years old, dating to the original addition of COLLATE support. The lack of field complaints suggests that there aren't a lot of user-visible consequences. We noticed the problem because it would trigger an assertion in DefineVirtualRelation if the invalid structure appears as an output column of a view; however, in a non-assert build, you don't see a crash just a (subtly incorrect) complaint about applying collation to a non-collatable type. I found that by putting the incorrect structure further down in a view, I could make a view definition that would fail dump/reload, per the added regression test case. But CollateExpr doesn't do anything at run-time, so this likely doesn't lead to any really exciting consequences. Per report from Yulin Pei. Back-patch to all supported branches. Discussion: https://postgr.es/m/HK0PR01MB22744393C474D503E16C8509F4709@HK0PR01MB2274.apcprd01.prod.exchangelabs.com
1 parent 6787e53 commit c402b02
Copy full SHA for c402b02

File tree

Expand file treeCollapse file tree

3 files changed

+39
-12
lines changed
Filter options
Expand file treeCollapse file tree

3 files changed

+39
-12
lines changed

‎src/backend/parser/parse_coerce.c

Copy file name to clipboardExpand all lines: src/backend/parser/parse_coerce.c
+18-11Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ coerce_to_target_type(ParseState *pstate, Node *expr, Oid exprtype,
9595
* *must* know that to avoid possibly calling hide_coercion_node on
9696
* something that wasn't generated by coerce_type. Note that if there are
9797
* multiple stacked CollateExprs, we just discard all but the topmost.
98+
* Also, if the target type isn't collatable, we discard the CollateExpr.
9899
*/
99100
origexpr = expr;
100101
while (expr && IsA(expr, CollateExpr))
@@ -114,7 +115,7 @@ coerce_to_target_type(ParseState *pstate, Node *expr, Oid exprtype,
114115
ccontext, cformat, location,
115116
(result != expr && !IsA(result, Const)));
116117

117-
if (expr != origexpr)
118+
if (expr != origexpr && type_is_collatable(targettype))
118119
{
119120
/* Reinstall top CollateExpr */
120121
CollateExpr *coll = (CollateExpr *) origexpr;
@@ -388,20 +389,26 @@ coerce_type(ParseState *pstate, Node *node,
388389
{
389390
/*
390391
* If we have a COLLATE clause, we have to push the coercion
391-
* underneath the COLLATE. This is really ugly, but there is little
392-
* choice because the above hacks on Consts and Params wouldn't happen
392+
* underneath the COLLATE; or discard the COLLATE if the target type
393+
* isn't collatable. This is really ugly, but there is little choice
394+
* because the above hacks on Consts and Params wouldn't happen
393395
* otherwise. This kluge has consequences in coerce_to_target_type.
394396
*/
395397
CollateExpr *coll = (CollateExpr *) node;
396-
CollateExpr *newcoll = makeNode(CollateExpr);
397398

398-
newcoll->arg = (Expr *)
399-
coerce_type(pstate, (Node *) coll->arg,
400-
inputTypeId, targetTypeId, targetTypeMod,
401-
ccontext, cformat, location);
402-
newcoll->collOid = coll->collOid;
403-
newcoll->location = coll->location;
404-
return (Node *) newcoll;
399+
result = coerce_type(pstate, (Node *) coll->arg,
400+
inputTypeId, targetTypeId, targetTypeMod,
401+
ccontext, cformat, location);
402+
if (type_is_collatable(targetTypeId))
403+
{
404+
CollateExpr *newcoll = makeNode(CollateExpr);
405+
406+
newcoll->arg = (Expr *) result;
407+
newcoll->collOid = coll->collOid;
408+
newcoll->location = coll->location;
409+
result = (Node *) newcoll;
410+
}
411+
return result;
405412
}
406413
pathtype = find_coercion_pathway(targetTypeId, inputTypeId, ccontext,
407414
&funcId);

‎src/test/regress/expected/collate.out

Copy file name to clipboardExpand all lines: src/test/regress/expected/collate.out
+15-1Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -688,13 +688,26 @@ SELECT collation for ((SELECT b FROM collate_test1 LIMIT 1));
688688
"C"
689689
(1 row)
690690

691+
-- old bug with not dropping COLLATE when coercing to non-collatable type
692+
CREATE VIEW collate_on_int AS
693+
SELECT c1+1 AS c1p FROM
694+
(SELECT ('4' COLLATE "C")::INT AS c1) ss;
695+
\d+ collate_on_int
696+
View "collate_tests.collate_on_int"
697+
Column | Type | Collation | Nullable | Default | Storage | Description
698+
--------+---------+-----------+----------+---------+---------+-------------
699+
c1p | integer | | | | plain |
700+
View definition:
701+
SELECT ss.c1 + 1 AS c1p
702+
FROM ( SELECT 4 AS c1) ss;
703+
691704
--
692705
-- Clean up. Many of these table names will be re-used if the user is
693706
-- trying to run any platform-specific collation tests later, so we
694707
-- must get rid of them.
695708
--
696709
DROP SCHEMA collate_tests CASCADE;
697-
NOTICE: drop cascades to 18 other objects
710+
NOTICE: drop cascades to 19 other objects
698711
DETAIL: drop cascades to table collate_test1
699712
drop cascades to table collate_test_like
700713
drop cascades to table collate_test2
@@ -713,3 +726,4 @@ drop cascades to table collate_test21
713726
drop cascades to table collate_test22
714727
drop cascades to collation mycoll2
715728
drop cascades to table collate_test23
729+
drop cascades to view collate_on_int

‎src/test/regress/sql/collate.sql

Copy file name to clipboardExpand all lines: src/test/regress/sql/collate.sql
+6Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,12 @@ SELECT collation for ('foo'::text);
266266
SELECT collation for ((SELECT a FROM collate_test1 LIMIT 1)); -- non-collatable type - error
267267
SELECT collation for ((SELECT b FROM collate_test1 LIMIT 1));
268268

269+
-- old bug with not dropping COLLATE when coercing to non-collatable type
270+
CREATE VIEW collate_on_int AS
271+
SELECT c1+1 AS c1p FROM
272+
(SELECT ('4' COLLATE "C")::INT AS c1) ss;
273+
\d+ collate_on_int
274+
269275

270276
--
271277
-- Clean up. Many of these table names will be re-used if the user is

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.