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

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