Re: BUG #16325: Assert failure on partitioning by int for a text value with a collation

The following bug has been logged on the website:

Bug reference: 16325
Logged by: Alexander Lakhin
Email address: exclusion(at)gmail(dot)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(at)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(at)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(at)entry=0x55cd3d2b1f5b "!(strvalue != ((void
*)0))",
errorType=errorType(at)entry=0x55cd3d0d9e88 "FailedAssertion",
fileName=fileName(at)entry=0x55cd3d2b1f50 "snprintf.c",
lineNumber=lineNumber(at)entry=442) at assert.c:54
#3 0x000055cd3d0d0903 in dopr (target=target(at)entry=0x7ffe43ec6310,
format=0x55cd3d1fbfcd "\"",
format(at)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(at)entry=1024,
fmt=fmt(at)entry=0x55cd3d1fbf70 "collation of partition bound value for
column \"%s\" does not match partition key collation \"%s\"",
args=args(at)entry=0x7ffe43ec63c0) at snprintf.c:195
#5 0x000055cd3d0d6e3c in pvsnprintf (buf=<optimized out>,
len=len(at)entry=1024,
fmt=fmt(at)entry=0x55cd3d1fbf70 "collation of partition bound value for
column \"%s\" does not match partition key collation \"%s\"",
args=args(at)entry=0x7ffe43ec63c0) at psprintf.c:110
#6 0x000055cd3ce1b357 in appendStringInfoVA (str=str(at)entry=0x7ffe43ec63a0,

fmt=fmt(at)entry=0x55cd3d1fbf70 "collation of partition bound value for
column \"%s\" does not match partition key collation \"%s\"",
args=args(at)entry=0x7ffe43ec63c0) at stringinfo.c:136
#7 0x000055cd3d087371 in errmsg (
fmt=fmt(at)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(at)entry=0x55cd3ed58148, val=0x55cd3ecbb1a8,
colName=colName(at)entry=0x55cd3ed58680 "i", colType=colType(at)entry=23,
colTypmod=colTypmod(at)entry=-1,
partCollation=partCollation(at)entry=0) at parse_utilcmd.c:4043
#9 0x000055cd3cd41231 in transformPartitionBound
(pstate=pstate(at)entry=0x55cd3ed58148,
parent=parent(at)entry=0x7f4e46f86290, spec=<optimized out>) at
parse_utilcmd.c:3802
#10 0x000055cd3cda67e4 in DefineRelation (stmt=stmt(at)entry=0x55cd3ed5a620,
relkind=relkind(at)entry=114 'r', ownerId=10,
ownerId(at)entry=0, typaddress=typaddress(at)entry=0x0,
queryString=queryString(at)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(at)entry=0x55cd3ed5a508, pstmt=pstmt(at)entry=0x55cd3ec99138,
queryString=queryString(at)entry=0x55cd3ec97fd8 "create table part_coll
partition of parted for values in ('1' collate \"POSIX\");",
context=context(at)entry=PROCESS_UTILITY_TOPLEVEL, params=params(at)entry=0x0,
queryEnv=queryEnv(at)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(at)entry=0x55cd3ec99138,
queryString=<optimized out>,
context=context(at)entry=PROCESS_UTILITY_TOPLEVEL, params=<optimized out>,
queryEnv=<optimized out>,
dest=dest(at)entry=0x55cd3ec99230, completionTag=0x7ffe43ec6d30 "") at
utility.c:360
#14 0x000055cd3cf5b026 in PortalRunUtility
(portal=portal(at)entry=0x55cd3ecff288, pstmt=pstmt(at)entry=0x55cd3ec99138,
isTopLevel=isTopLevel(at)entry=true,
setHoldSnapshot=setHoldSnapshot(at)entry=false, dest=dest(at)entry=0x55cd3ec99230,

completionTag=completionTag(at)entry=0x7ffe43ec6d30 "") at pquery.c:1175
#15 0x000055cd3cf5bc70 in PortalRunMulti
(portal=portal(at)entry=0x55cd3ecff288, isTopLevel=isTopLevel(at)entry=true,
setHoldSnapshot=setHoldSnapshot(at)entry=false,
dest=dest(at)entry=0x55cd3ec99230, altdest=altdest(at)entry=0x55cd3ec99230,
completionTag=completionTag(at)entry=0x7ffe43ec6d30 "") at pquery.c:1321
#16 0x000055cd3cf5c9df in PortalRun (portal=portal(at)entry=0x55cd3ecff288,
count=count(at)entry=9223372036854775807,
isTopLevel=isTopLevel(at)entry=true, run_once=run_once(at)entry=true,
dest=dest(at)entry=0x55cd3ec99230,
altdest=altdest(at)entry=0x55cd3ec99230, completionTag=0x7ffe43ec6d30 "")
at pquery.c:796
#17 0x000055cd3cf58cc3 in exec_simple_query (
query_string=query_string(at)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(at)entry=0x55cd3ecc3388, dbname=<optimized out>,
username=<optimized out>) at postgres.c:4247
#19 0x000055cd3ceccf37 in BackendRun (port=port(at)entry=0x55cd3ecbba60) at
postmaster.c:4437
#20 0x000055cd3ced0229 in BackendStartup (port=port(at)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.

> 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

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

> 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.

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

> 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.

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

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

> 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!