Thread: Core dump happens when execute sql CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10));

Core dump happens when execute sql CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10));

From

Yulin PEI

Date:

12 April 2021, 18:39:38

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

Re: Core dump happens when execute sql CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10));

From

Tom Lane

Date:

12 April 2021, 19:59:43

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")::INTFROM 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

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

回复: Core dump happens when execute sql CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10));

From

Yulin PEI

Date:

13 April 2021, 11:27:20

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

回复: Core dump happens when execute sql CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10));

From

Yulin PEI

Date:

13 April 2021, 12:27:08

I think it is better to add this test case to regress.

Attachment

0001-add-regress.patch

回复: Core dump happens when execute sql CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM generate_series(1, 10));

From

Yulin PEI

Date:

18 April 2021, 19:31:07

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.

Attachment

fix-collation-coercion.patch