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

On Tue, Jul 9, 2019 at 3:31 PM Mike Palmiotto
<mike(dot)palmiotto(at)crunchydata(dot)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