Core dump happens when execute sql CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10));
HI hackers,
I found it could cause a crash when executing sql statement: `CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10)); ` in postgres 13.2 release.
The crash happens at view.c:89 and I did some analysis:
```
ColumnDef *def = makeColumnDef(tle->resname,
exprType((Node *) tle->expr),
exprTypmod((Node *) tle->expr),
exprCollation((Node *) tle->expr));
/*
* It's possible that the column is of a collatable type but the
* collation could not be resolved, so double-check.
*/
// Here is the analysis:
//example : ('4' COLLATE "C")::INT
//exprCollation((Node *) tle->expr) is the oid of collate "COLLATE 'C'" so def->collOid is valid
//exprType((Node *) tle->expr)) is 23 which is the oid of type int4.
//We know that int4 is not collatable by calling type_is_collatable()
if (type_is_collatable(exprType((Node *) tle->expr)))
{
if (!OidIsValid(def->collOid))
ereport(ERROR,
(errcode(ERRCODE_INDETERMINATE_COLLATION),
errmsg("could not determine which collation to use for view column \"%s\"",
def->colname),
errhint("Use the COLLATE clause to set the collation explicitly.")));
}
else
// So we are here! int is not collatable and def->collOid is valid.
Assert(!OidIsValid(def->collOid));
```
I am not sure whether to fix this bug in function DefineVirtualRelation or to fix this bug in parse tree and analyze procedure, so maybe we can discuss.
Best Regard!
Yulin PEI
Yulin PEI <ypeiae@connect.ust.hk> writes:
I found it could cause a crash when executing sql statement: `CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10)); ` in postgres 13.2 release.
Nice catch. I don't think the code in DefineVirtualRelation is wrong:
exprCollation shouldn't report any collation for an expression of a
non-collatable type. Rather the problem is with an old kluge in
coerce_type(), which will push a type coercion underneath a CollateExpr
... without any mind for the possibility that the coercion result isn't
collatable. So the right fix is more or less the attached.
regards, tom lane
Attachments:
fix-bogus-collation-coercion.patchtext/x-diff; charset=us-ascii; name=fix-bogus-collation-coercion.patchDownload
diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index d5310f27db..228ee8e7d6 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -95,6 +95,7 @@ coerce_to_target_type(ParseState *pstate, Node *expr, Oid exprtype,
* *must* know that to avoid possibly calling hide_coercion_node on
* something that wasn't generated by coerce_type. Note that if there are
* multiple stacked CollateExprs, we just discard all but the topmost.
+ * Also, if the target type isn't collatable, we discard the CollateExpr.
*/
origexpr = expr;
while (expr && IsA(expr, CollateExpr))
@@ -114,7 +115,7 @@ coerce_to_target_type(ParseState *pstate, Node *expr, Oid exprtype,
ccontext, cformat, location,
(result != expr && !IsA(result, Const)));
- if (expr != origexpr)
+ if (expr != origexpr && type_is_collatable(targettype))
{
/* Reinstall top CollateExpr */
CollateExpr *coll = (CollateExpr *) origexpr;
@@ -384,7 +385,7 @@ coerce_type(ParseState *pstate, Node *node,
if (result)
return result;
}
- if (IsA(node, CollateExpr))
+ if (IsA(node, CollateExpr) && type_is_collatable(targetTypeId))
{
/*
* If we have a COLLATE clause, we have to push the coercion
After reading the code and the patch, I think the patch is good. If the type is non-collatable, we do not add a CollateExpr node as a 'parent' node to the coerced node.
________________________________
发件人: Tom Lane <tgl@sss.pgh.pa.us>
发送时间: 2021年4月13日 0:59
收件人: Yulin PEI <ypeiae@connect.ust.hk>
抄送: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
主题: Re: Core dump happens when execute sql CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10));
Yulin PEI <ypeiae@connect.ust.hk> writes:
I found it could cause a crash when executing sql statement: `CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10)); ` in postgres 13.2 release.
Nice catch. I don't think the code in DefineVirtualRelation is wrong:
exprCollation shouldn't report any collation for an expression of a
non-collatable type. Rather the problem is with an old kluge in
coerce_type(), which will push a type coercion underneath a CollateExpr
... without any mind for the possibility that the coercion result isn't
collatable. So the right fix is more or less the attached.
regards, tom lane
I think it is better to add this test case to regress.
________________________________
发件人: Tom Lane <tgl@sss.pgh.pa.us>
发送时间: 2021年4月13日 0:59
收件人: Yulin PEI <ypeiae@connect.ust.hk>
抄送: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
主题: Re: Core dump happens when execute sql CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10));
Yulin PEI <ypeiae@connect.ust.hk> writes:
I found it could cause a crash when executing sql statement: `CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10)); ` in postgres 13.2 release.
Nice catch. I don't think the code in DefineVirtualRelation is wrong:
exprCollation shouldn't report any collation for an expression of a
non-collatable type. Rather the problem is with an old kluge in
coerce_type(), which will push a type coercion underneath a CollateExpr
... without any mind for the possibility that the coercion result isn't
collatable. So the right fix is more or less the attached.
regards, tom lane
Attachments:
0001-add-regress.patchapplication/octet-stream; name=0001-add-regress.patchDownload
From 432fe83b4305eaf716b331c66ab6c221eb6708a4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E8=A3=B4=E7=8E=89=E6=9E=97?= <peiyulin@bytedance.com>
Date: Tue, 13 Apr 2021 17:23:57 +0800
Subject: [PATCH] add regress
---
src/test/regress/expected/create_cast.out | 9 +++++++++
src/test/regress/sql/create_cast.sql | 6 ++++++
2 files changed, 15 insertions(+)
diff --git a/src/test/regress/expected/create_cast.out b/src/test/regress/expected/create_cast.out
index 88f94a6..2ca0fb8 100644
--- a/src/test/regress/expected/create_cast.out
+++ b/src/test/regress/expected/create_cast.out
@@ -72,3 +72,12 @@ SELECT 1234::int4::casttesttype; -- Should work now
foo1234
(1 row)
+-- Test coerce a non-collatable type with collation
+CREATE VIEW cast_with_collation(c1) AS (SELECT ('1' COLLATE "C")::INT FROM generate_series(1, 10));
+select count(*) from cast_with_collation;
+ count
+-------
+ 10
+(1 row)
+
+DROP VIEW cast_with_collation;
diff --git a/src/test/regress/sql/create_cast.sql b/src/test/regress/sql/create_cast.sql
index b11cf88..3888857 100644
--- a/src/test/regress/sql/create_cast.sql
+++ b/src/test/regress/sql/create_cast.sql
@@ -52,3 +52,9 @@ $$ SELECT ('foo'::text || $1::text)::casttesttype; $$;
CREATE CAST (int4 AS casttesttype) WITH FUNCTION int4_casttesttype(int4) AS IMPLICIT;
SELECT 1234::int4::casttesttype; -- Should work now
+
+
+-- Test coerce a non-collatable type with collation
+CREATE VIEW cast_with_collation(c1) AS (SELECT ('1' COLLATE "C")::INT FROM generate_series(1, 10));
+select count(*) from cast_with_collation;
+DROP VIEW cast_with_collation;
\ No newline at end of file
--
2.23.0
After several tests, I found that this patch do not fix the bug well.
I think we should use the same logic to treat parent CollateExpr and child CollateExpr. In your patch, if the parent node is CollateExpr and the target type is non-collatable, we coerce CollateExpr->arg. If the child node is CollateExpr and the target type is non-collatable, we just skip.
Some types can be casted to another type even if type_is_collatable returns false. Like bytea to int (It depends on the content of the string). If we simply skip, bytea will never be casted to int even if the content is all digits.
So the attachment is my patch and it works well as far as I tested.
________________________________
发件人: Tom Lane <tgl@sss.pgh.pa.us>
发送时间: 2021年4月13日 0:59
收件人: Yulin PEI <ypeiae@connect.ust.hk>
抄送: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
主题: Re: Core dump happens when execute sql CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10));
Yulin PEI <ypeiae@connect.ust.hk> writes:
I found it could cause a crash when executing sql statement: `CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10)); ` in postgres 13.2 release.
Nice catch. I don't think the code in DefineVirtualRelation is wrong:
exprCollation shouldn't report any collation for an expression of a
non-collatable type. Rather the problem is with an old kluge in
coerce_type(), which will push a type coercion underneath a CollateExpr
... without any mind for the possibility that the coercion result isn't
collatable. So the right fix is more or less the attached.
regards, tom lane
Attachments:
fix-collation-coercion.patchapplication/octet-stream; name=fix-collation-coercion.patchDownload
diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index 2ffe470..08ca348 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -1,3 +1,4 @@
+
/*-------------------------------------------------------------------------
*
* parse_coerce.c
@@ -94,6 +95,7 @@ coerce_to_target_type(ParseState *pstate, Node *expr, Oid exprtype,
* *must* know that to avoid possibly calling hide_coercion_node on
* something that wasn't generated by coerce_type. Note that if there are
* multiple stacked CollateExprs, we just discard all but the topmost.
+ * Also, if the target type isn't collatable, we discard the CollateExpr.
*/
origexpr = expr;
while (expr && IsA(expr, CollateExpr))
@@ -113,7 +115,7 @@ coerce_to_target_type(ParseState *pstate, Node *expr, Oid exprtype,
ccontext, cformat, location,
(result != expr && !IsA(result, Const)));
- if (expr != origexpr)
+ if (expr != origexpr && type_is_collatable(targettype))
{
/* Reinstall top CollateExpr */
CollateExpr *coll = (CollateExpr *) origexpr;
@@ -381,6 +383,7 @@ coerce_type(ParseState *pstate, Node *node,
if (result)
return result;
}
+
if (IsA(node, CollateExpr))
{
/*
@@ -392,6 +395,13 @@ coerce_type(ParseState *pstate, Node *node,
CollateExpr *coll = (CollateExpr *) node;
CollateExpr *newcoll = makeNode(CollateExpr);
+
+ if (!type_is_collatable(targetTypeId)) {
+ return coerce_type(pstate, (Node *) coll->arg,
+ inputTypeId, targetTypeId, targetTypeMod,
+ ccontext, cformat, location);
+ }
+
newcoll->arg = (Expr *)
coerce_type(pstate, (Node *) coll->arg,
inputTypeId, targetTypeId, targetTypeMod,
@@ -2890,3 +2900,4 @@ typeIsOfTypedTable(Oid reltypeId, Oid reloftypeId)
return result;
}
+
--
2.11.0
Yulin PEI <ypeiae@connect.ust.hk> writes:
After several tests, I found that this patch do not fix the bug well.
What do you think is wrong with it?
So the attachment is my patch and it works well as far as I tested.
This seems equivalent to the already-committed patch [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=c402b02b9fb53aee2a26876de90a8f95f9a9be92 except that
it wastes a makeNode call in the coerce-to-uncollatable-type case.
regards, tom lane
Consider the SQL statement 'SELECT (('1' COLLATE "C") ||(B'1'));' . Intuitively, the result will be '11' and the result is '11' in pg 13.2 release as well.
The function stack is make_fn_arguments -> coerce_type, which means that the param "Node *node" of function coerce_type could be a CollateExpr Node.
Let's look at your patch:
```
// node is ('1' COLLATE "C")
// targetType is varbit and it is non-collatable
if (IsA(node, CollateExpr) && type_is_collatable(targetTypeId))
{
// we will not reach here.
CollateExpr *coll = (CollateExpr *) node;
CollateExpr *newcoll = makeNode(CollateExpr);
....
// An error will be generated. "failed to find conversion function"
}
```
So I suggest:
```
// node is ('1' COLLATE "C")
if (IsA(node, CollateExpr))
{
CollateExpr *coll = (CollateExpr *) node;
CollateExpr *newcoll = makeNode(CollateExpr);
//targetType is varbit and it is non-collatable
if (!type_is_collatable(targetTypeId)) {
// try to convert '1'(string) to varbit
// We do not make a new CollateExpr here, but don't forget to coerce coll->arg.
return coerce_type(pstate, (Node *) coll->arg,
inputTypeId, targetTypeId, targetTypeMod,
ccontext, cformat, location);
}
...
}
```
________________________________
寄件者: Tom Lane <tgl@sss.pgh.pa.us>
寄件日期: 2021年4月19日 1:46
收件者: Yulin PEI <ypeiae@connect.ust.hk>
副本: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
主旨: Re: 回复: Core dump happens when execute sql CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10));
Yulin PEI <ypeiae@connect.ust.hk> writes:
After several tests, I found that this patch do not fix the bug well.
What do you think is wrong with it?
So the attachment is my patch and it works well as far as I tested.
This seems equivalent to the already-committed patch [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=c402b02b9fb53aee2a26876de90a8f95f9a9be92 except that
it wastes a makeNode call in the coerce-to-uncollatable-type case.
regards, tom lane
Yulin PEI <ypeiae@connect.ust.hk> writes:
Let's look at your patch:
```
// node is ('1' COLLATE "C")
// targetType is varbit and it is non-collatable
if (IsA(node, CollateExpr) && type_is_collatable(targetTypeId))
{
// we will not reach here.
That's not the committed patch, though. I realized after posting
it that it didn't maintain the same behavior in coerce_type as
coerce_to_target_type. But the actually-committed fix does, and
as I said, what you're suggesting seems equivalent though a bit
messier.
regards, tom lane