Thread
Commits
-
Fix crash when using COLLATE in partition bound expressions
- 0a9ae1800f67 12.3 landed
- c0187869a0f6 13.0 landed
-
BUG #16325: Assert failure on partitioning by int for a text value with a collation
The Post Office <noreply@postgresql.org> — 2020-03-28T05:43:05Z
The following bug has been logged on the website: Bug reference: 16325 Logged by: Alexander Lakhin Email address: exclusion@gmail.com PostgreSQL version: 12.2 Operating system: Ubuntu 18.04 Description: The following query: create table parted (i int) partition by list (i); create table part_coll partition of parted for values in ('1' collate "POSIX"); leads to an assert failure with the following stack trace: Core was generated by `postgres: law regression [local] CREATE TABLE '. Program terminated with signal SIGABRT, Aborted. #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51 51 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. (gdb) bt #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51 #1 0x00007f4e455fa801 in __GI_abort () at abort.c:79 #2 0x000055cd3d082f40 in ExceptionalCondition ( conditionName=conditionName@entry=0x55cd3d2b1f5b "!(strvalue != ((void *)0))", errorType=errorType@entry=0x55cd3d0d9e88 "FailedAssertion", fileName=fileName@entry=0x55cd3d2b1f50 "snprintf.c", lineNumber=lineNumber@entry=442) at assert.c:54 #3 0x000055cd3d0d0903 in dopr (target=target@entry=0x7ffe43ec6310, format=0x55cd3d1fbfcd "\"", format@entry=0x55cd3d1fbf70 "collation of partition bound value for column \"%s\" does not match partition key collation \"%s\"", args=0x7ffe43ec63c0) at snprintf.c:442 #4 0x000055cd3d0d0ff4 in pg_vsnprintf (str=<optimized out>, count=<optimized out>, count@entry=1024, fmt=fmt@entry=0x55cd3d1fbf70 "collation of partition bound value for column \"%s\" does not match partition key collation \"%s\"", args=args@entry=0x7ffe43ec63c0) at snprintf.c:195 #5 0x000055cd3d0d6e3c in pvsnprintf (buf=<optimized out>, len=len@entry=1024, fmt=fmt@entry=0x55cd3d1fbf70 "collation of partition bound value for column \"%s\" does not match partition key collation \"%s\"", args=args@entry=0x7ffe43ec63c0) at psprintf.c:110 #6 0x000055cd3ce1b357 in appendStringInfoVA (str=str@entry=0x7ffe43ec63a0, fmt=fmt@entry=0x55cd3d1fbf70 "collation of partition bound value for column \"%s\" does not match partition key collation \"%s\"", args=args@entry=0x7ffe43ec63c0) at stringinfo.c:136 #7 0x000055cd3d087371 in errmsg ( fmt=fmt@entry=0x55cd3d1fbf70 "collation of partition bound value for column \"%s\" does not match partition key collation \"%s\"") at elog.c:794 #8 0x000055cd3cd3e3a4 in transformPartitionBoundValue (pstate=pstate@entry=0x55cd3ed58148, val=0x55cd3ecbb1a8, colName=colName@entry=0x55cd3ed58680 "i", colType=colType@entry=23, colTypmod=colTypmod@entry=-1, partCollation=partCollation@entry=0) at parse_utilcmd.c:4043 #9 0x000055cd3cd41231 in transformPartitionBound (pstate=pstate@entry=0x55cd3ed58148, parent=parent@entry=0x7f4e46f86290, spec=<optimized out>) at parse_utilcmd.c:3802 #10 0x000055cd3cda67e4 in DefineRelation (stmt=stmt@entry=0x55cd3ed5a620, relkind=relkind@entry=114 'r', ownerId=10, ownerId@entry=0, typaddress=typaddress@entry=0x0, queryString=queryString@entry=0x55cd3ec97fd8 "create table part_coll partition of parted for values in ('1' collate \"POSIX\");") at tablecmds.c:972 #11 0x000055cd3cf5ed12 in ProcessUtilitySlow (pstate=pstate@entry=0x55cd3ed5a508, pstmt=pstmt@entry=0x55cd3ec99138, queryString=queryString@entry=0x55cd3ec97fd8 "create table part_coll partition of parted for values in ('1' collate \"POSIX\");", context=context@entry=PROCESS_UTILITY_TOPLEVEL, params=params@entry=0x0, queryEnv=queryEnv@entry=0x0, dest=0x55cd3ec99230, completionTag=0x7ffe43ec6d30 "") at utility.c:1014 #12 0x000055cd3cf5eac6 in standard_ProcessUtility (pstmt=0x55cd3ec99138, queryString=0x55cd3ec97fd8 "create table part_coll partition of parted for values in ('1' collate \"POSIX\");", context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x55cd3ec99230, completionTag=0x7ffe43ec6d30 "") at utility.c:927 #13 0x000055cd3cf5eb74 in ProcessUtility (pstmt=pstmt@entry=0x55cd3ec99138, queryString=<optimized out>, context=context@entry=PROCESS_UTILITY_TOPLEVEL, params=<optimized out>, queryEnv=<optimized out>, dest=dest@entry=0x55cd3ec99230, completionTag=0x7ffe43ec6d30 "") at utility.c:360 #14 0x000055cd3cf5b026 in PortalRunUtility (portal=portal@entry=0x55cd3ecff288, pstmt=pstmt@entry=0x55cd3ec99138, isTopLevel=isTopLevel@entry=true, setHoldSnapshot=setHoldSnapshot@entry=false, dest=dest@entry=0x55cd3ec99230, completionTag=completionTag@entry=0x7ffe43ec6d30 "") at pquery.c:1175 #15 0x000055cd3cf5bc70 in PortalRunMulti (portal=portal@entry=0x55cd3ecff288, isTopLevel=isTopLevel@entry=true, setHoldSnapshot=setHoldSnapshot@entry=false, dest=dest@entry=0x55cd3ec99230, altdest=altdest@entry=0x55cd3ec99230, completionTag=completionTag@entry=0x7ffe43ec6d30 "") at pquery.c:1321 #16 0x000055cd3cf5c9df in PortalRun (portal=portal@entry=0x55cd3ecff288, count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true, dest=dest@entry=0x55cd3ec99230, altdest=altdest@entry=0x55cd3ec99230, completionTag=0x7ffe43ec6d30 "") at pquery.c:796 #17 0x000055cd3cf58cc3 in exec_simple_query ( query_string=query_string@entry=0x55cd3ec97fd8 "create table part_coll partition of parted for values in ('1' collate \"POSIX\");") at postgres.c:1215 #18 0x000055cd3cf5ac67 in PostgresMain (argc=<optimized out>, argv=argv@entry=0x55cd3ecc3388, dbname=<optimized out>, username=<optimized out>) at postgres.c:4247 #19 0x000055cd3ceccf37 in BackendRun (port=port@entry=0x55cd3ecbba60) at postmaster.c:4437 #20 0x000055cd3ced0229 in BackendStartup (port=port@entry=0x55cd3ecbba60) at postmaster.c:4128 #21 0x000055cd3ced0545 in ServerLoop () at postmaster.c:1704 #22 0x000055cd3ced195e in PostmasterMain (argc=3, argv=<optimized out>) at postmaster.c:1377 #23 0x000055cd3ce2cdf4 in main (argc=3, argv=0x55cd3ec92630) at main.c:228 Reproduced on REL_12..master. -
Re: BUG #16325: Assert failure on partitioning by int for a text value with a collation
Dmitry Dolgov <9erthalion6@gmail.com> — 2020-03-28T12:53:03Z
> On Sat, Mar 28, 2020 at 05:43:05AM +0000, PG Bug reporting form wrote: > > > The following query: > create table parted (i int) partition by list (i); > create table part_coll partition of parted for values in ('1' collate > "POSIX"); > > leads to an assert failure with the following stack trace: Thanks for reporting! Looks like transformPartitionBoundValue needs to check that the source collumn is collatable, then we get more expected result: =# create table part_coll partition of parted for values in ('1' collate "POSIX"); ERROR: 42804: collations are not supported by type integer -
Re: BUG #16325: Assert failure on partitioning by int for a text value with a collation
Michael Paquier <michael@paquier.xyz> — 2020-04-01T07:08:42Z
On Sat, Mar 28, 2020 at 01:53:03PM +0100, Dmitry Dolgov wrote: > Thanks for reporting! Looks like transformPartitionBoundValue needs to > check that the source collumn is collatable, then we get more expected > result: > > =# create table part_coll partition of parted for values in ('1' collate "POSIX"); > ERROR: 42804: collations are not supported by type integer We have here a side effect of 7c079d74, that has given the possibility to pass down general expressions for partition bounds. > + if (type_is_collatable(colType)) > + { > + if (!OidIsValid(exprCollOid)) > + ereport(ERROR, > + (errcode(ERRCODE_INDETERMINATE_COLLATION), > + errmsg("could not determine which collation to use for partition expression"), > + errhint("Use the COLLATE clause to set the collation explicitly."))); > + } > + else > + { > + if (OidIsValid(exprCollOid)) > + ereport(ERROR, > + (errcode(ERRCODE_DATATYPE_MISMATCH), > + errmsg("collations are not supported by type %s", > + format_type_be(colType)))); > + } > + It seems to me that the first error cannot happen, and should not happen. When the expression is transformed, there is a lookup at the collation used via transformCollateClause() -> LookupCollation() -> get_collation_oid() which would never return an invalid OID if the collation exists, throwing an error instead. The reason why a similar check exists in tablecmds.c is that there can be a collation override, no? -- Michael -
Re: BUG #16325: Assert failure on partitioning by int for a text value with a collation
Dmitry Dolgov <9erthalion6@gmail.com> — 2020-04-01T12:01:17Z
> On Wed, Apr 01, 2020 at 04:08:42PM +0900, Michael Paquier wrote: > > On Sat, Mar 28, 2020 at 01:53:03PM +0100, Dmitry Dolgov wrote: > > Thanks for reporting! Looks like transformPartitionBoundValue needs to > > check that the source collumn is collatable, then we get more expected > > result: > > > > =# create table part_coll partition of parted for values in ('1' collate "POSIX"); > > ERROR: 42804: collations are not supported by type integer > > We have here a side effect of 7c079d74, that has given the possibility > to pass down general expressions for partition bounds. Yep. That's why I've placed a test for this fix close to those introduced in this commit. > > + if (type_is_collatable(colType)) > > + { > > + if (!OidIsValid(exprCollOid)) > > + ereport(ERROR, > > + (errcode(ERRCODE_INDETERMINATE_COLLATION), > > + errmsg("could not determine which collation to use for partition expression"), > > + errhint("Use the COLLATE clause to set the collation explicitly."))); > > + } > > + else > > + { > > + if (OidIsValid(exprCollOid)) > > + ereport(ERROR, > > + (errcode(ERRCODE_DATATYPE_MISMATCH), > > + errmsg("collations are not supported by type %s", > > + format_type_be(colType)))); > > + } > > + > > It seems to me that the first error cannot happen, and should not > happen. When the expression is transformed, there is a lookup at the > collation used via transformCollateClause() -> LookupCollation() -> > get_collation_oid() which would never return an invalid OID if the > collation exists, throwing an error instead. The reason why a similar > check exists in tablecmds.c is that there can be a collation override, > no? Good point, I believe you're right. This means that the fix can be reduced as in attachments. -
Re: BUG #16325: Assert failure on partitioning by int for a text value with a collation
Michael Paquier <michael@paquier.xyz> — 2020-04-02T02:01:50Z
On Wed, Apr 01, 2020 at 02:01:17PM +0200, Dmitry Dolgov wrote: > On Wed, Apr 01, 2020 at 04:08:42PM +0900, Michael Paquier wrote: >> It seems to me that the first error cannot happen, and should not >> happen. When the expression is transformed, there is a lookup at the >> collation used via transformCollateClause() -> LookupCollation() -> >> get_collation_oid() which would never return an invalid OID if the >> collation exists, throwing an error instead. The reason why a similar >> check exists in tablecmds.c is that there can be a collation override, >> no? > > Good point, I believe you're right. This means that the fix can be > reduced as in attachments. Thanks. > + /* > + * No need to check if exprCollOid is invalid, since if it would be, > + * then transformCollateClause will throw an error earlier > + */ > + if (!type_is_collatable(colType)) > + ereport(ERROR, > + (errcode(ERRCODE_DATATYPE_MISMATCH), > + errmsg("collations are not supported by type %s", > + format_type_be(colType)))); > + > > if (OidIsValid(exprCollOid) && > exprCollOid != DEFAULT_COLLATION_OID && > exprCollOid != partCollation) Another point that could be made is that as we know that we are in the presence of a CollateExpr() here, and exprCollOid will never be InvalidOid. So there could be a point in using an assertion instead of the check done below making sure that exprCollOid is always valid? -- Michael -
Re: BUG #16325: Assert failure on partitioning by int for a text value with a collation
Dmitry Dolgov <9erthalion6@gmail.com> — 2020-04-03T15:29:10Z
> On Thu, Apr 02, 2020 at 11:01:50AM +0900, Michael Paquier wrote: > > > + /* > > + * No need to check if exprCollOid is invalid, since if it would be, > > + * then transformCollateClause will throw an error earlier > > + */ > > + if (!type_is_collatable(colType)) > > + ereport(ERROR, > > + (errcode(ERRCODE_DATATYPE_MISMATCH), > > + errmsg("collations are not supported by type %s", > > + format_type_be(colType)))); > > + > > > > if (OidIsValid(exprCollOid) && > > exprCollOid != DEFAULT_COLLATION_OID && > > exprCollOid != partCollation) > > Another point that could be made is that as we know that we are in the > presence of a CollateExpr() here, and exprCollOid will never be > InvalidOid. So there could be a point in using an assertion instead > of the check done below making sure that exprCollOid is always valid? Thanks for the suggestion. It's not strictly related to the fix I guess, but of course you're right, and we can indeed improve this part and replace the check with an assert. -
Re: BUG #16325: Assert failure on partitioning by int for a text value with a collation
Michael Paquier <michael@paquier.xyz> — 2020-04-07T07:49:44Z
On Fri, Apr 03, 2020 at 05:29:10PM +0200, Dmitry Dolgov wrote: > Thanks for the suggestion. It's not strictly related to the fix I guess, > but of course you're right, and we can indeed improve this part and > replace the check with an assert. Thanks for the new version of the patch. I have been playing a bit today, and finished with the version attached. Some comments have been tweaked, and I have moved the test in the block with the other invalid cases for consistency, adding an extra test able to trigger a similar error within the transformation step where a parse context exists. It is a bit late now so I cannot push that today, but I'll do that tomorrow. -- Michael
-
Re: BUG #16325: Assert failure on partitioning by int for a text value with a collation
Michael Paquier <michael@paquier.xyz> — 2020-04-08T06:06:39Z
On Tue, Apr 07, 2020 at 04:49:44PM +0900, Michael Paquier wrote: > Thanks for the new version of the patch. I have been playing a bit > today, and finished with the version attached. Some comments have > been tweaked, and I have moved the test in the block with the other > invalid cases for consistency, adding an extra test able to trigger a > similar error within the transformation step where a parse context > exists. It is a bit late now so I cannot push that today, but I'll > do that tomorrow. I was considering this stuff again, and I got cold feet regarding the fact that we may be able to hit the case of a collation not set if the type is collatable depending on how the restrictions now in place for partition bound expressions get lifted in the future, so I have committed what you sent as v1, after changing the first error message to use "partition bound expression" and after adding a comment on top of the new checks. -- Michael
-
Re: BUG #16325: Assert failure on partitioning by int for a text value with a collation
Dmitry Dolgov <9erthalion6@gmail.com> — 2020-04-08T08:20:32Z
> On Wed, Apr 08, 2020 at 03:06:39PM +0900, Michael Paquier wrote: > > I was considering this stuff again, and I got cold feet regarding the > fact that we may be able to hit the case of a collation not set if the > type is collatable depending on how the restrictions now in place for > partition bound expressions get lifted in the future, so I have > committed what you sent as v1, after changing the first error message > to use "partition bound expression" and after adding a comment on top > of the new checks. Thanks!