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

On Tue, Jul 9, 2019 at 3:31 PM Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
> Attached you will find a patch (rebased on master) that passes all
> tests on my local CentOS 7 box. Thanks again for the catch!

Hi Mike,

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...
 */

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?

+/* Macro to use partitionChildAccess_hook. Handles NULL-checking. */

It's not a macro, it's a function.

+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.

+        return (*partitionChildAccess_hook) (childOID);

The syntax we usually use for calling function pointers is just
partitionChildAccess_hook(childOID).

-- 
Thomas Munro
https://enterprisedb.com