From: | Simon Riggs <simon(at)2ndquadrant(dot)com> |
---|---|
To: | Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Peter Geoghegan <pg(at)bowt(dot)ie>, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, Robert Haas <robertmhaas(at)gmail(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com>, Pavan Deolasee <pavan(dot)deolasee(at)2ndquadrant(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [HACKERS] MERGE SQL Statement for PG11 |
Date: | 2018-04-05 07:55:50 |
Message-ID: | CANP8+j+4mdmdzKc80HoZkaP5f3SZGMOvNyYCoU7j6ZqM5h9Q1g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 5 April 2018 at 07:01, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Konstantin Knizhnik | 2018-04-05 08:03:58 | Re: Postgres stucks in deadlock detection |
Previous Message | Amit Langote | 2018-04-05 07:31:35 | Re: [HACKERS] Add support for tuple routing to foreign partitions |