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
