Обсуждение: 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@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.

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