On 2017/09/15 1:37, Robert Haas wrote:
> On Thu, Sep 14, 2017 at 7:56 AM, Amit Khandekar
> wrote:
>> On 14 September 2017 at 06:43, Amit Langote
>>> langote_amit...@lab.ntt.co.jp> wrote:
>>> Attached updated patch.
>>
>> @@ -1222,151 +1209,130 @@ PartitionDispatch *
>> RelationGetPartitionDispat
On Thu, Sep 14, 2017 at 7:56 AM, Amit Khandekar wrote:
> On 14 September 2017 at 06:43, Amit Langote
>> langote_amit...@lab.ntt.co.jp> wrote:
>> Attached updated patch.
>
> @@ -1222,151 +1209,130 @@ PartitionDispatch *
> RelationGetPartitionDispatchInfo(Relation rel,
>
On 14 September 2017 at 06:43, Amit Langote
> langote_amit...@lab.ntt.co.jp> wrote:
> Attached updated patch.
@@ -1222,151 +1209,130 @@ PartitionDispatch *
RelationGetPartitionDispatchInfo(Relation rel,
int
*num_parted, List **leaf_
On 2017/09/14 1:42, Robert Haas wrote:
> On Wed, Sep 13, 2017 at 6:02 AM, Amit Langote
> wrote:
>> It seems to me we don't really need the first patch all that much. That
>> is, let's keep PartitionDispatchData the way it is for now, since we don't
>> really have any need for it beside tuple-rout
On Wed, Sep 13, 2017 at 6:02 AM, Amit Langote
wrote:
> It seems to me we don't really need the first patch all that much. That
> is, let's keep PartitionDispatchData the way it is for now, since we don't
> really have any need for it beside tuple-routing (EIBO as committed didn't
> need it for on
On 13 September 2017 at 15:32, Amit Langote
wrote:
> On 2017/09/11 18:56, Amit Langote wrote:
>> Attached updated patch does it that way for both partitioned table indexes
>> and leaf partition indexes. Thanks for pointing it out.
>
> It seems to me we don't really need the first patch all that m
On 2017/09/11 18:56, Amit Langote wrote:
> Attached updated patch does it that way for both partitioned table indexes
> and leaf partition indexes. Thanks for pointing it out.
It seems to me we don't really need the first patch all that much. That
is, let's keep PartitionDispatchData the way it
Hi Amit,
On 2017/09/11 16:16, Amit Khandekar wrote:
> Thanks Amit for the patch. I am still reviewing it, but meanwhile
> below are a few comments so far ...
Thanks for the review.
> + next_parted_idx += (list_length(*pds) - next_parted_idx - 1);
>
> I think this can be replaced just by :
> +
Thanks Amit for the patch. I am still reviewing it, but meanwhile
below are a few comments so far ...
On 8 September 2017 at 15:53, Amit Langote
wrote:
> [PATCH 2/2] Make RelationGetPartitionDispatch expansion order
> depth-first
>
> This is so as it matches what the planner is doing with partit
On 2017/09/05 14:11, Amit Khandekar wrote:
> Great, thanks. Just wanted to make sure someone is working on that,
> because, as you said, it is no longer an EIBO patch. Since you are
> doing that, I won't work on that.
Here is that patch (actually two patches). Sorry it took me a bit.
Description
On 4 September 2017 at 06:34, Amit Langote
wrote:
> Hi Amit,
>
> On 2017/09/03 16:07, Amit Khandekar wrote:
>> On 31 August 2017 at 13:06, Amit Langote
>> wrote:
Mind you, that idea has some problems anyway in the face of default
partitions, null partitions, and list partitions which a
Hi Amit,
On 2017/09/03 16:07, Amit Khandekar wrote:
> On 31 August 2017 at 13:06, Amit Langote
> wrote:
>>> Mind you, that idea has some problems anyway in the face of default
>>> partitions, null partitions, and list partitions which accept
>>> non-contiguous values (e.g. one partition for 1, 3
On 31 August 2017 at 13:06, Amit Langote wrote:
>> Mind you, that idea has some problems anyway in the face of default
>> partitions, null partitions, and list partitions which accept
>> non-contiguous values (e.g. one partition for 1, 3, 5; another for 2,
>> 4, 6). We might need to mark the Part
On Thu, Aug 31, 2017 at 2:56 AM, Ashutosh Bapat
wrote:
> Here are the patches revised a bit. I have esp changed the variable
> names and arguments to reflect their true role in the functions. Also
> updated prologue of expand_single_inheritance_child() to mention
> "has_child". Let me know if thos
On Thu, Aug 31, 2017 at 3:36 AM, Amit Langote
wrote:
> ISTM, the primary motivation for the EIBO patch at this point is to get
> the partitions ordered in a predictable manner so that the partition-wise
> join patch and update partition key patches could implement certain logic
> using O (n) algor
On 2017/08/31 4:45, Robert Haas wrote:
> On Wed, Aug 30, 2017 at 12:47 PM, Ashutosh Bapat
> wrote:
>> +1. I think we should just pull out the OIDs from partition descriptor.
>
> Like this? The first patch refactors the expansion of a single child
> out into a separate function, and the second pa
On Thu, Aug 31, 2017 at 1:15 AM, Robert Haas wrote:
> On Wed, Aug 30, 2017 at 12:47 PM, Ashutosh Bapat
> wrote:
>> +1. I think we should just pull out the OIDs from partition descriptor.
>
> Like this? The first patch refactors the expansion of a single child
> out into a separate function, and
On Wed, Aug 30, 2017 at 12:47 PM, Ashutosh Bapat
wrote:
> +1. I think we should just pull out the OIDs from partition descriptor.
Like this? The first patch refactors the expansion of a single child
out into a separate function, and the second patch implements EIBO on
top of it.
I realized whil
On Wed, Aug 30, 2017 at 9:42 PM, Robert Haas wrote:
> On Wed, Aug 30, 2017 at 6:08 AM, Amit Khandekar
> wrote:
>> On 25 August 2017 at 23:58, Robert Haas wrote:
>>> That just leaves indexes. In a world where keystate, tupslot, and
>>> tupmap are removed from the PartitionDispatchData, you must
On Wed, Aug 30, 2017 at 6:08 AM, Amit Khandekar wrote:
> On 25 August 2017 at 23:58, Robert Haas wrote:
>> That just leaves indexes. In a world where keystate, tupslot, and
>> tupmap are removed from the PartitionDispatchData, you must need
>> indexes or there would be no point in constructing a
On Wed, Aug 30, 2017 at 10:31 AM, Robert Haas wrote:
> On Wed, Aug 30, 2017 at 9:22 AM, Ashutosh Bapat
> wrote:
>> Amit's patches seem to be addressing the third point here. But the
>> expansion is not happening in breadth-first manner. We are expanding
>> all the partitioned partitions first and
On Wed, Aug 30, 2017 at 9:22 AM, Ashutosh Bapat
wrote:
> Amit's patches seem to be addressing the third point here. But the
> expansion is not happening in breadth-first manner. We are expanding
> all the partitioned partitions first and then leaf partitions. So
> that's not exactly "breadth-first
On Wed, Aug 30, 2017 at 8:33 AM, Robert Haas wrote:
> On Tue, Aug 29, 2017 at 10:36 PM, Amit Langote
> wrote:
>>> I keep having the feeling that this is a big patch with a small patch
>>> struggling to get out. Is it really necessary to change
>>> RelationGetPartitionDispatchInfo so much or coul
On 25 August 2017 at 23:58, Robert Haas wrote:
>
> That just leaves indexes. In a world where keystate, tupslot, and
> tupmap are removed from the PartitionDispatchData, you must need
> indexes or there would be no point in constructing a
> PartitionDispatchData object in the first place; any app
On 2017/08/30 12:03, Robert Haas wrote:
> On Tue, Aug 29, 2017 at 10:36 PM, Amit Langote
> wrote:
>>> I keep having the feeling that this is a big patch with a small patch
>>> struggling to get out. Is it really necessary to change
>>> RelationGetPartitionDispatchInfo so much or could you just do
On Tue, Aug 29, 2017 at 10:36 PM, Amit Langote
wrote:
>> I keep having the feeling that this is a big patch with a small patch
>> struggling to get out. Is it really necessary to change
>> RelationGetPartitionDispatchInfo so much or could you just do a really
>> minimal surgery to remove the code
On 2017/08/29 4:26, Robert Haas wrote:
> I think this patch could be further simplified by continuing to use
> the existing function signature for RelationGetPartitionDispatchInfo
> instead of changing it to return a List * rather than an array. I
> don't see any benefit to such a change. The cur
On Mon, Aug 28, 2017 at 6:38 AM, Amit Langote
wrote:
> I am worried about the open relcache reference in PartitionDispatch when
> we start using it in the planner. Whereas there is a ExecEndModifyTable()
> as a suitable place to close that reference, there doesn't seem to exist
> one within the p
On 2017/08/26 3:28, Robert Haas wrote:
> On Mon, Aug 21, 2017 at 2:10 AM, Amit Langote
> wrote:
>> [ new patches ]
>
> I am failing to understand the point of separating PartitionDispatch
> into PartitionDispatch and PartitionTableInfo. That seems like an
> unnecessary multiplication of entities
On Mon, Aug 21, 2017 at 2:10 AM, Amit Langote
wrote:
> [ new patches ]
I am failing to understand the point of separating PartitionDispatch
into PartitionDispatch and PartitionTableInfo. That seems like an
unnecessary multiplication of entities, as does the introduction of
PartitionKeyInfo. I a
On 2017/08/21 13:11, Ashutosh Bapat wrote:
> On Sat, Aug 19, 2017 at 1:21 AM, Robert Haas wrote:
>> On Fri, Aug 18, 2017 at 1:17 AM, Ashutosh Bapat
>> wrote:
>>> 0004 patch in partition-wise join patchset has code to expand
>>> partition hierarchy. That patch is expanding inheritance hierarchy in
Hi Amit,
On 2017/08/17 21:18, Amit Khandekar wrote:
> Anyways, some more comments :
>
> In ExecSetupPartitionTupleRouting(), not sure why ptrinfos array is an
> array of pointers. Why can't it be an array of
> PartitionTupleRoutingInfo structure rather than pointer to that
> structure ?
AFAIK,
On Sat, Aug 19, 2017 at 1:21 AM, Robert Haas wrote:
> On Fri, Aug 18, 2017 at 1:17 AM, Ashutosh Bapat
> wrote:
>> 0004 patch in partition-wise join patchset has code to expand
>> partition hierarchy. That patch is expanding inheritance hierarchy in
>> depth first manner. Robert commented that ins
On Fri, Aug 18, 2017 at 1:17 AM, Ashutosh Bapat
wrote:
> 0004 patch in partition-wise join patchset has code to expand
> partition hierarchy. That patch is expanding inheritance hierarchy in
> depth first manner. Robert commented that instead of depth first
> manner, it will be better if we expand
On 18 August 2017 at 10:55, Amit Langote wrote:
> On 2017/08/18 4:54, Robert Haas wrote:
>> On Thu, Aug 17, 2017 at 8:39 AM, Ashutosh Bapat
>> wrote:
>>> [2] had a patch with some changes to the original patch you posted. I
>>> didn't describe those changes in my mail, since they rearranged the
>
On 2017/08/18 4:54, Robert Haas wrote:
> On Thu, Aug 17, 2017 at 8:39 AM, Ashutosh Bapat
> wrote:
>> [2] had a patch with some changes to the original patch you posted. I
>> didn't describe those changes in my mail, since they rearranged the
>> comments. Those changes are not part of this patch an
On Fri, Aug 18, 2017 at 10:32 AM, Amit Khandekar wrote:
>
> I think in the final changes after applying all 3 patches, the
> redundant tuple slot is no longer present. But ...
>> We don't really need the PartitionDispatch objects either,
>> except for the OIDs they contain. There's a lot of extra
On 18 August 2017 at 01:24, Robert Haas wrote:
> On Thu, Aug 17, 2017 at 8:39 AM, Ashutosh Bapat
> wrote:
>> [2] had a patch with some changes to the original patch you posted. I
>> didn't describe those changes in my mail, since they rearranged the
>> comments. Those changes are not part of this
On Fri, Aug 18, 2017 at 10:12 AM, Amit Langote
wrote
>
>>> 0002: Teach expand_inherited_rtentry to use partition bound order
>>
>> 0004 in [1] expands a multi-level partition hierarchy into similar
>> inheritance hierarchy. That patch doesn't need all OIDs in one go. It
>> will have to handle the
Hi Ashutosh,
On 2017/08/17 21:39, Ashutosh Bapat wrote:
> On Thu, Aug 17, 2017 at 12:59 PM, Amit Langote
> wrote:
>>
>> Attached rest of the patches. 0001 has changes per Ashutosh's review
>> comments [2]:
>>
>> 0001: Relieve RelationGetPartitionDispatchInfo() of doing any locking
>
> [2] had a
On Thu, Aug 17, 2017 at 8:39 AM, Ashutosh Bapat
wrote:
> [2] had a patch with some changes to the original patch you posted. I
> didn't describe those changes in my mail, since they rearranged the
> comments. Those changes are not part of this patch and you haven't
> comments about those changes a
On Thu, Aug 17, 2017 at 12:59 PM, Amit Langote
wrote:
>
> Attached rest of the patches. 0001 has changes per Ashutosh's review
> comments [2]:
>
> 0001: Relieve RelationGetPartitionDispatchInfo() of doing any locking
[2] had a patch with some changes to the original patch you posted. I
didn't de
On 17 August 2017 at 06:39, Amit Langote wrote:
> Hi Amit,
>
> Thanks for the comments.
>
> On 2017/08/16 20:30, Amit Khandekar wrote:
>> On 16 August 2017 at 11:06, Amit Langote
>> wrote:
>>
>>> Attached updated patches.
>>
>> Thanks Amit for the patches.
>>
>> I too agree with the overall appr
On 2017/08/17 10:09, Amit Langote wrote:
> On 2017/08/16 20:30, Amit Khandekar wrote:
>> On 16 August 2017 at 11:06, Amit Langote
>> wrote:
>> I am not
>> sure whether we are planning to commit these two things together or
>> incrementally :
>> 1. Expand in bound order
>> 2. Allow for locking onl
On Thu, Aug 17, 2017 at 10:54 AM, Amit Langote
wrote:
> On 2017/08/17 13:56, Ashutosh Bapat wrote:
>> On Thu, Aug 17, 2017 at 8:06 AM, Amit Langote
>> wrote:
>>> On 2017/08/17 11:22, Robert Haas wrote:
On Wed, Aug 16, 2017 at 10:12 PM, Amit Langote
wrote:
>> In the catalogs we are
On 2017/08/17 13:56, Ashutosh Bapat wrote:
> On Thu, Aug 17, 2017 at 8:06 AM, Amit Langote
> wrote:
>> On 2017/08/17 11:22, Robert Haas wrote:
>>> On Wed, Aug 16, 2017 at 10:12 PM, Amit Langote
>>> wrote:
> In the catalogs we are using full "partitioned" e.g.
> pg_partitioned_table. May
On Thu, Aug 17, 2017 at 8:06 AM, Amit Langote
wrote:
> On 2017/08/17 11:22, Robert Haas wrote:
>> On Wed, Aug 16, 2017 at 10:12 PM, Amit Langote
>> wrote:
In the catalogs we are using full "partitioned" e.g. pg_partitioned_table.
May
be we should name the column as "inhchildpartit
On 2017/08/17 11:22, Robert Haas wrote:
> On Wed, Aug 16, 2017 at 10:12 PM, Amit Langote
> wrote:
>>> In the catalogs we are using full "partitioned" e.g. pg_partitioned_table.
>>> May
>>> be we should name the column as "inhchildpartitioned".
>>
>> Sure.
>
> I suggest inhpartitioned or inhispar
On Wed, Aug 16, 2017 at 10:12 PM, Amit Langote
wrote:
>> In the catalogs we are using full "partitioned" e.g. pg_partitioned_table.
>> May
>> be we should name the column as "inhchildpartitioned".
>
> Sure.
I suggest inhpartitioned or inhispartition. inhchildpartitioned seems too long.
> There
Hi Ashutosh,
Thanks for the review and the updated patch.
On 2017/08/16 21:48, Ashutosh Bapat wrote:
> On Wed, Aug 16, 2017 at 11:06 AM, Amit Langote
> wrote:
>
>>
>>> This patch series is blocking a bunch of other things, so it would be
>>> nice if you could press forward with this quickly.
>>
Hi Amit,
Thanks for the comments.
On 2017/08/16 20:30, Amit Khandekar wrote:
> On 16 August 2017 at 11:06, Amit Langote
> wrote:
>
>> Attached updated patches.
>
> Thanks Amit for the patches.
>
> I too agree with the overall approach taken for keeping the locking
> order consistent: it's be
On Wed, Aug 16, 2017 at 11:06 AM, Amit Langote
wrote:
>
>> This patch series is blocking a bunch of other things, so it would be
>> nice if you could press forward with this quickly.
>
> Attached updated patches.
>
Review for 0001. The attached patch has some minor changes to the
comments and co
On 16 August 2017 at 11:06, Amit Langote wrote:
> Attached updated patches.
Thanks Amit for the patches.
I too agree with the overall approach taken for keeping the locking
order consistent: it's best to do the locking with the existing
find_all_inheritors() since it is much cheaper than to loc
Thanks for the review.
On 2017/08/16 2:27, Robert Haas wrote:
> On Wed, Aug 9, 2017 at 10:11 PM, Amit Langote
> wrote:
>>> P.S. While I haven't reviewed 0002 in detail, I think the concept of
>>> minimizing what needs to be built in RelationGetPartitionDispatchInfo
>>> is a very good idea.
>>
>>
On 2017/08/10 18:52, Beena Emerson wrote:
> Hi Amit,
>
> On Thu, Aug 10, 2017 at 7:41 AM, Amit Langote
> wrote:
>> On 2017/08/05 2:25, Robert Haas wrote:
>>> Concretely, my proposal is:
>>>
>>> P.S. While I haven't reviewed 0002 in detail, I think the concept of
>>> minimizing what needs to be bu
On Wed, Aug 9, 2017 at 10:11 PM, Amit Langote
wrote:
>> P.S. While I haven't reviewed 0002 in detail, I think the concept of
>> minimizing what needs to be built in RelationGetPartitionDispatchInfo
>> is a very good idea.
>
> I put this patch ahead in the list and so it's now 0001.
I think what y
Hi Amit,
On Thu, Aug 10, 2017 at 7:41 AM, Amit Langote
wrote:
> On 2017/08/05 2:25, Robert Haas wrote:
>> Concretely, my proposal is:
>>
>> P.S. While I haven't reviewed 0002 in detail, I think the concept of
>> minimizing what needs to be built in RelationGetPartitionDispatchInfo
>> is a very go
On 2017/08/05 2:25, Robert Haas wrote:
> Concretely, my proposal is:
>
> 1. Before calling RelationGetPartitionDispatchInfo, the calling code
> should use find_all_inheritors to lock all the relevant relations (or
> the planner could use find_all_inheritors to get a list of relation
> OIDs, store
On 2017/08/08 4:34, Robert Haas wrote:
> On Mon, Aug 7, 2017 at 2:54 AM, Amit Langote
> wrote:
>> As long as find_all_inheritors() is a place only to determine the order in
>> which partitions will be locked, it's fine. My concern is about the time
>> of actual locking, which in the current plann
On 2017/08/07 14:37, Amit Khandekar wrote:
> On 4 August 2017 at 22:55, Robert Haas wrote:
>> P.S. While I haven't reviewed 0002 in detail, I think the concept of
>> minimizing what needs to be built in RelationGetPartitionDispatchInfo
>> is a very good idea.
>
> True. I also think, RelationGetPa
On Mon, Aug 7, 2017 at 2:54 AM, Amit Langote
wrote:
> I think Amit Khandekar mentioned this on the UPDATE partition key thread [1].
Yes, similar discussion.
> As long as find_all_inheritors() is a place only to determine the order in
> which partitions will be locked, it's fine. My concern is a
On Mon, Aug 7, 2017 at 1:48 AM, Ashutosh Bapat
wrote:
> with the way schema is designed. May be it's better to use your idea
> of using get_rel_relkind() or find a way to check that the flag is in
> sync with the relkind, like when building the relcache.
That's got the same problem as building a
On 2017/08/05 2:25, Robert Haas wrote:
> On Fri, Aug 4, 2017 at 3:38 AM, Amit Langote
> wrote:
>> The current way to expand inherited tables, including partitioned tables,
>> is to use either find_all_inheritors() or find_inheritance_children()
>> depending on the context. They return child table
On Mon, Aug 7, 2017 at 11:18 AM, Ashutosh Bapat
wrote:
>>
>> One objection to this line of attack is that there might be a good
>> case for locking only the partitioned inheritors first and then going
>> back and locking the leaf nodes in a second pass, or even only when
>> required for a particul
On Fri, Aug 4, 2017 at 10:55 PM, Robert Haas wrote:
> On Fri, Aug 4, 2017 at 3:38 AM, Amit Langote
> wrote:
>> The current way to expand inherited tables, including partitioned tables,
>> is to use either find_all_inheritors() or find_inheritance_children()
>> depending on the context. They retu
On 2017/08/04 20:28, Ashutosh Bapat wrote:
> On Fri, Aug 4, 2017 at 1:08 PM, Amit Langote
> wrote:
>> The current way to expand inherited tables, including partitioned tables,
>> is to use either find_all_inheritors() or find_inheritance_children()
>> depending on the context. They return child t
On 4 August 2017 at 22:55, Robert Haas wrote:
>
> 1. Before calling RelationGetPartitionDispatchInfo, the calling code
> should use find_all_inheritors to lock all the relevant relations (or
> the planner could use find_all_inheritors to get a list of relation
> OIDs, store it in the plan in order
On Fri, Aug 4, 2017 at 3:38 AM, Amit Langote
wrote:
> The current way to expand inherited tables, including partitioned tables,
> is to use either find_all_inheritors() or find_inheritance_children()
> depending on the context. They return child table OIDs in the (ascending)
> order of those OIDs
On Fri, Aug 4, 2017 at 1:08 PM, Amit Langote
wrote:
> The current way to expand inherited tables, including partitioned tables,
> is to use either find_all_inheritors() or find_inheritance_children()
> depending on the context. They return child table OIDs in the (ascending)
> order of those OIDs
The current way to expand inherited tables, including partitioned tables,
is to use either find_all_inheritors() or find_inheritance_children()
depending on the context. They return child table OIDs in the (ascending)
order of those OIDs, which means the callers that need to lock the child
tables
70 matches
Mail list logo
