On 5 April 2018 at 07:01, Pavan Deolasee <pavan.deola...@gmail.com> wrote:
>> +/*
>> + * Given OID of the partition leaf, return the index of the leaf in the
>> + * partition hierarchy.
>> + */
>> +int
>> +ExecFindPartitionByOid(PartitionTupleRouting *proute, Oid partoid)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < proute->num_partitions; i++)
>> + {
>> + if (proute->partition_oids[i] == partoid)
>> + break;
>> + }
>> +
>> + Assert(i < proute->num_partitions);
>> + return i;
>> +}
>>
>> Shouldn't we at least warn in a comment that this is O(N)? And document
>> that it does weird stuff if the OID isn't found?
>
>
> Yeah, added a comment. Also added a ereport(ERROR) if we do not find the
> partition. There was already an Assert, but may be ERROR is better.
>
>>
>>
>> Perhaps just introduce a PARTOID syscache?
>>
>
> Probably as a separate patch. Anything more than a handful partitions is
> anyways known to be too slow and I doubt this code will add anything
> material impact to that.
There's a few others trying to change that now, so I think we should
consider working on this now.
PARTOID syscache sounds like a good approach.
>> diff --git a/src/backend/executor/nodeMerge.c
>> b/src/backend/executor/nodeMerge.c
>> new file mode 100644
>> index 00000000000..0e0d0795d4d
>> --- /dev/null
>> +++ b/src/backend/executor/nodeMerge.c
>> @@ -0,0 +1,575 @@
>>
>> +/*-------------------------------------------------------------------------
>> + *
>> + * nodeMerge.c
>> + * routines to handle Merge nodes relating to the MERGE command
>>
>> Isn't this file misnamed and it should be execMerge.c? The rest of the
>> node*.c files are for nodes that are invoked via execProcnode /
>> ExecProcNode(). This isn't an actual executor node.
>
>
> Makes sense. Done. (Now that the patch is committed, I don't know if there
> would be a rethink about changing file names. May be not, just raising that
> concern)
My review notes suggest a file called execMerge.c. I didn't spot the
filename change.
I think it's important to do that because there is no executor node
called Merge. That is especially confusing because there *is* an
executor node called MergeAppend and we want some cognitive distance
between those things.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
