Re: COLLATE: Hash partition vs UPDATE

Thanks for the review.

On 2019/04/15 5:50, Tom Lane wrote:
> Jesper Pedersen <jesper.pedersen@redhat.com> writes:
>> Yeah, that works here - apart from an issue with the test case; fixed in 
>> the attached.
> 
> Couple issues spotted in an eyeball review of that:
> 
> * There is code that supposes that partsupfunc[] is the last
> field of ColumnsHashData, eg
> 
>             fcinfo->flinfo->fn_extra =
>                 MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt,
>                                        offsetof(ColumnsHashData, partsupfunc) +
>                                        sizeof(FmgrInfo) * nargs);
> 
> I'm a bit surprised that this patch manages to run without crashing,
> because this would certainly not allocate space for partcollid[].
> 
> I think we would likely be well advised to do
> 
> -        FmgrInfo    partsupfunc[PARTITION_MAX_KEYS];
> +        FmgrInfo    partsupfunc[FLEXIBLE_ARRAY_MEMBER];

I went with this:

-        FmgrInfo    partsupfunc[PARTITION_MAX_KEYS];
         Oid         partcollid[PARTITION_MAX_KEYS];
+        FmgrInfo    partsupfunc[FLEXIBLE_ARRAY_MEMBER];

> to make it more obvious that that has to be the last field.  Or else
> drop the cuteness with variable-size allocations of ColumnsHashData.
> FmgrInfo is only 48 bytes, I'm not really sure that it's worth the
> risk of bugs to "optimize" this.

I wonder if workloads on hash partitioned tables that require calling
satisfies_hash_partition repeatedly may not be as common as thought when
writing this code?  The only case I see where it's being repeatedly called
is bulk inserts into a hash-partitioned table, that too, only if BR
triggers on partitions necessitate rechecking the partition constraint.

> * I see collation-less calls of the partsupfunc at both partbounds.c:2931
> and partbounds.c:2970, but this patch touches only the first one.  How
> can that be right?

Oops, that's wrong.

Attached updated patch.

Thanks,
Amit