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
