Re: [RFC] [PATCH] Flexible "partition pruning" hook

On Thu, Jul 11, 2019 at 8:49 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> <snip>
>
> Here are some comments on superficial aspects of the patch:
>
> +/* Custom partition child access hook. Provides further partition pruning given
> + * child OID.
> + */
>
> Should be like:
>
> /*
>  * Multi-line comment...
>  */

Fixed in attached patch.

>
> Why "child"?  Don't you really mean "Partition pruning hook.  Provides
> custom pruning given partition OID." or something?
>
> +typedef bool (*partitionChildAccess_hook_type) (Oid childOID);
> +PGDLLIMPORT partitionChildAccess_hook_type partitionChildAccess_hook;
>
> Hmm, I wonder if this could better evoke the job that it's doing...
> partition_filter_hook?
> partition_access_filter_hook?  partition_prune_hook?

Ended up going with partition_prune_hook. Good call.

>
> +/* Macro to use partitionChildAccess_hook. Handles NULL-checking. */
>
> It's not a macro, it's a function.

Copy-pasta. Fixed.

>
> +static inline bool InvokePartitionChildAccessHook (Oid childOID)
> +{
> +    if (partitionChildAccess_hook && enable_partition_pruning && childOID)
> +    {
>
> Normally we write OidIsValid(childOID) rather than comparing with 0.
> I wonder if you should call the variable relId?  Single line if
> branches don't usually get curly braces.

Fixed.

>
> +        return (*partitionChildAccess_hook) (childOID);
>
> The syntax we usually use for calling function pointers is just
> partitionChildAccess_hook(childOID).

Fixed.

-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com