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
