Horiguchi-san,
Thanks for taking a look. Replying to all your emails here.
On 2017/11/10 12:30, Kyotaro HORIGUCHI wrote:
> In 0002, bms_add_range has a bit naive-looking loop
>
> + while (wordnum <= uwordnum)
> + {
> + bitmapword mask = (bitmapword) ~0;
> +
> + /
On 10 November 2017 at 16:30, Kyotaro HORIGUCHI
wrote:
> In 0002, bms_add_range has a bit naive-looking loop
>
> + while (wordnum <= uwordnum)
> + {
> + bitmapword mask = (bitmapword) ~0;
> +
> + /* If working on the lower word, zero out bits below 'lower'.
Hello,
At Fri, 10 Nov 2017 14:44:55 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI
wrote in
<20171110.144455.117208639.horiguchi.kyot...@lab.ntt.co.jp>
> > Those two conditions are not orthogonal. Maybe something like
> > following seems more understantable.
> >
> > > if (!constfalse)
> > > {
Ooops! The following comment is wrong. Please ignore it.
At Fri, 10 Nov 2017 14:38:11 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI
wrote in
<20171110.143811.97616847.horiguchi.kyot...@lab.ntt.co.jp>
> Those two conditions are not orthogonal. Maybe something like
> following seems more underst
Hello, this is the second part of the review.
At Fri, 10 Nov 2017 12:30:00 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI
wrote in
<20171110.123000.151902771.horiguchi.kyot...@lab.ntt.co.jp>
> In 0002, bms_add_range has a bit naive-looking loop
> In 0003,
In 0004,
The name get_partitions_fro
Hello,
At Fri, 10 Nov 2017 09:34:57 +0900, Amit Langote
wrote in
<5fcb1a9f-b4ad-119d-14c7-282c30c7f...@lab.ntt.co.jp>
> Hi Amul.
>
> On 2017/11/09 20:05, amul sul wrote:
> > On Mon, Nov 6, 2017 at 3:31 PM, Amit Langote
> > wrote:
> >> On 2017/11/06 14:32, David Rowley wrote:
> >>> On 6 Novemb
Hi Amul.
On 2017/11/09 20:05, amul sul wrote:
> On Mon, Nov 6, 2017 at 3:31 PM, Amit Langote
> wrote:
>> On 2017/11/06 14:32, David Rowley wrote:
>>> On 6 November 2017 at 17:30, Amit Langote wrote:
On 2017/11/03 13:32, David Rowley wrote:
> On 31 October 2017 at 21:43, Amit Langote wrot
On Mon, Nov 6, 2017 at 3:31 PM, Amit Langote
wrote:
> On 2017/11/06 14:32, David Rowley wrote:
>> On 6 November 2017 at 17:30, Amit Langote wrote:
>>> On 2017/11/03 13:32, David Rowley wrote:
On 31 October 2017 at 21:43, Amit Langote wrote:
[]
>
> Attached updated set of patches, includin
Hi Rajkumar,
Thanks for testing.
On 2017/11/08 15:52, Rajkumar Raghuwanshi wrote:
> On Mon, Nov 6, 2017 at 3:31 PM, Amit Langote
> wrote:
>
>> Attached updated set of patches, including the fix to make the new pruning
>> code handle Boolean partitioning.
>>
>
> Hi Amit,
>
> I have tried pruni
On Mon, Nov 6, 2017 at 3:31 PM, Amit Langote
wrote:
> Attached updated set of patches, including the fix to make the new pruning
> code handle Boolean partitioning.
>
Hi Amit,
I have tried pruning for different values of constraint exclusion GUC
change, not sure exactly how it should behave, bu
Hi David.
Thanks for the review.
(..also looking at the comments you sent earlier today.)
On 2017/11/07 11:14, David Rowley wrote:
> On 7 November 2017 at 01:52, David Rowley
> wrote:
>> Thanks. I'll look over it all again starting my Tuesday morning. (UTC+13)
>
> I have a little more review
On 7 November 2017 at 01:52, David Rowley wrote:
> Thanks. I'll look over it all again starting my Tuesday morning. (UTC+13)
Hi Amit,
I had another look over this today. Apologies if any of the review seems petty.
Here goes:
1. If test seems to be testing for a child that's a partitioned table
On 2017/11/06 21:52, David Rowley wrote:
> On 6 November 2017 at 23:01, Amit Langote
> wrote:
>> OK, I have gotten rid of the min/max partition index interface and instead
>> adopted the bms_add_range() approach by including your patch to add the
>> same in the patch set (which is now 0002 in the
On 7 November 2017 at 01:52, David Rowley wrote:
> Thanks. I'll look over it all again starting my Tuesday morning. (UTC+13)
I have a little more review to share:
1. Missing "in" in comment. Should be "mentioned in"
* get_append_rel_partitions
* Return the list of partitions of rel that pass
On 6 November 2017 at 23:01, Amit Langote wrote:
> OK, I have gotten rid of the min/max partition index interface and instead
> adopted the bms_add_range() approach by including your patch to add the
> same in the patch set (which is now 0002 in the whole set). I have to
> admit that it's simpler
On 2017/11/06 13:15, David Rowley wrote:
> On 31 October 2017 at 21:43, Amit Langote
> wrote:
>> Attached updated version of the patches
>
> match_clauses_to_partkey() needs to allow for the way quals on Bool
> columns are represented.
>
> create table pt (a bool not null) partition by list (a)
On 6 November 2017 at 17:30, Amit Langote wrote:
> On 2017/11/03 13:32, David Rowley wrote:
>> On 31 October 2017 at 21:43, Amit Langote
>> wrote:
>> 1. This comment seem wrong.
>>
>> /*
>> * Since the clauses in rel->baserestrictinfo should all contain Const
>> * operands, it should be possible
On 2017/11/06 12:53, David Rowley wrote:
> On 3 November 2017 at 17:32, David Rowley
> wrote:
>> 2. This code is way more complex than it needs to be.
>>
>> if (num_parts > 0)
>> {
>> int j;
>>
>> all_indexes = (int *) palloc(num_parts * sizeof(int));
>> j = 0;
>> if (min_part_idx >= 0 && max_par
On 31 October 2017 at 21:43, Amit Langote wrote:
> Attached updated version of the patches
match_clauses_to_partkey() needs to allow for the way quals on Bool
columns are represented.
create table pt (a bool not null) partition by list (a);
create table pt_true partition of pt for values in('t')
On 3 November 2017 at 17:32, David Rowley wrote:
> 2. This code is way more complex than it needs to be.
>
> if (num_parts > 0)
> {
> int j;
>
> all_indexes = (int *) palloc(num_parts * sizeof(int));
> j = 0;
> if (min_part_idx >= 0 && max_part_idx >= 0)
> {
> for (i = min_part_idx; i <= max_part_
On 31 October 2017 at 21:43, Amit Langote wrote:
> Attached updated version of the patches addressing some of your comments
I've spent a bit of time looking at these but I'm out of time for now.
So far I have noted down the following;
1. This comment seem wrong.
/*
* Since the clauses in rel->
On 31 October 2017 at 21:43, Amit Langote wrote:
> Attached updated version of the patches addressing some of your comments
> above and fixing a bug that Rajkumar reported [1]. As mentioned there,
> I'm including here a patch (the 0005 of the attached) to tweak the default
> range partition const
Thanks for the test case.
On 2017/10/30 17:09, Rajkumar Raghuwanshi wrote:
> I am getting wrong output when default is sub-partitioned further, below is
> a test case.
>
> CREATE TABLE lpd(a int, b varchar, c float) PARTITION BY LIST (a);
> CREATE TABLE lpd_p1 PARTITION OF lpd FOR VALUES IN (1,2,
On Mon, Oct 30, 2017 at 12:20 PM, Amit Langote
wrote:
> On 2017/10/30 14:55, Amit Langote wrote:
>> Fixed in the attached updated patch, along with a new test in 0001 to
>> cover this case. Also, made a few tweaks to 0003 and 0005 (moved some
>> code from the former to the latter) around the hand
On Mon, Oct 30, 2017 at 12:20 PM, Amit Langote <
langote_amit...@lab.ntt.co.jp> wrote:
> In the previous versions, RT index of the table needed to be passed to
> partition.c, which I realized is no longer needed, so I removed that
> requirement from the interface. As a result, patches 0002 and 00
On Fri, Oct 27, 2017 at 2:41 PM, Amit Langote wrote:
> 0001: added some new tests
> 0002: no change
> 0003: fixed issue that Rajkumar reported (cope with Params properly)
> 0004: no change
> 0005: fix the case to prune the default partition when warranted (the
> issue reported by Beena)
>
On Thu, Oct 26, 2017 at 4:47 PM, Amit Langote wrote:
>
> Meanwhile, attached updated set of patches including fixes for the typos
> you reported in the other message. Updated 0005 fixes the first bug (the
> Case 1 in your email), while other patches 0002-0004 are updated mostly to
> fix the repo
On 2017/10/27 13:57, Robert Haas wrote:
> On Fri, Oct 27, 2017 at 3:17 AM, Amit Langote
> wrote:
>>> I don't think we really want to get into theorem-proving here, because
>>> it's slow.
>>
>> Just to be clear, I'm saying we could use theorem-proving (if at all) just
>> for the default partition.
On Fri, Oct 27, 2017 at 3:17 AM, Amit Langote
wrote:
>> I don't think we really want to get into theorem-proving here, because
>> it's slow.
>
> Just to be clear, I'm saying we could use theorem-proving (if at all) just
> for the default partition.
I don't really see why it should be needed there
On 2017/10/26 20:34, Robert Haas wrote:
> On Thu, Oct 26, 2017 at 1:17 PM, Amit Langote
> wrote:
>> It can perhaps taught to not make that conclusion by taking into account
>> the default partition's partition constraint, which includes constraint
>> inherited from the parent, viz. 1 <= col1 < 500
On Thu, Oct 26, 2017 at 1:17 PM, Amit Langote
wrote:
> It can perhaps taught to not make that conclusion by taking into account
> the default partition's partition constraint, which includes constraint
> inherited from the parent, viz. 1 <= col1 < 50001. To do that, it might
> be possible to summ
Hello,
On Wed, Oct 25, 2017 at 1:07 PM, Amit Langote
wrote:
> On 2017/10/25 15:47, Amit Langote wrote:
>> On 2017/10/24 1:38, Beena Emerson wrote:
>>> I had noticed this and also that this crash:
>>>
>>> tprt PARTITION BY RANGE(Col1)
>>>tprt_1 FOR VALUES FROM (1) TO (50001) PARTITION BY R
Hello Amit,
Thanks for the updated patches
On Wed, Oct 25, 2017 at 1:07 PM, Amit Langote
wrote:
> On 2017/10/25 15:47, Amit Langote wrote:
>> On 2017/10/24 1:38, Beena Emerson wrote:
>>> I had noticed this and also that this crash:
>>>
>>> tprt PARTITION BY RANGE(Col1)
>>>tprt_1 FOR VALU
On Mon, Oct 23, 2017 at 3:24 PM, Rajkumar Raghuwanshi
wrote:
>
> On Mon, Oct 23, 2017 at 1:12 PM, Amit Langote
> wrote:
>>
>> The compiler I have here (gcc (GCC) 6.2.0) didn't complain like that for
>> this typedef redefinition introduced by the 0002 patch, but it seems that
>> it's not needed an
On Mon, Oct 23, 2017 at 1:12 PM, Amit Langote wrote:
> The compiler I have here (gcc (GCC) 6.2.0) didn't complain like that for
> this typedef redefinition introduced by the 0002 patch, but it seems that
> it's not needed anyway, so got rid of that line in the attached updated
> patch.
>
> Fixed
On Thu, Oct 19, 2017 at 12:16 PM, Amit Langote <
langote_amit...@lab.ntt.co.jp> wrote:
> Description of the attached patches:
>
> 0001: add new tests for partition-pruning
>
> 0002: patch that makes all the changes needed in the planer (adds a stub
> function in partition.c)
>
> 0003: patch
Hi David.
Thanks a lot for your review comments and sorry it took me a while to reply.
On 2017/09/28 18:16, David Rowley wrote:
> On 27 September 2017 at 14:22, Amit Langote
> wrote:
>> - 0001 includes refactoring that Dilip proposed upthread [1] (added him as
>> an author). I slightly tweake
On Sun, Oct 1, 2017 at 9:13 PM, Amit Langote
wrote:
> I agree. Equality checks are going to be common enough to warrant them to
> be handled specially, instead of implementing equality-pruning on top of
> min/max framework.
What you might do is pass and
optionally allow a second . Then for
the
On 2017/09/30 1:28, Robert Haas wrote:
> On Thu, Sep 28, 2017 at 5:16 AM, David Rowley
> wrote:
>> I'd imagine, for
>> each partition key, you'd want to store a Datum with the minimum and
>> maximum possible value based on the quals processed. If either the
>> minimum or maximum is still set to NU
On Thu, Sep 28, 2017 at 5:16 AM, David Rowley
wrote:
> I'd imagine, for
> each partition key, you'd want to store a Datum with the minimum and
> maximum possible value based on the quals processed. If either the
> minimum or maximum is still set to NULL, then it's unbounded at that
> end. If you e
On 27 September 2017 at 14:22, Amit Langote
wrote:
> - 0001 includes refactoring that Dilip proposed upthread [1] (added him as
> an author). I slightly tweaked his patch -- renamed the function
> get_matching_clause to match_clauses_to_partkey, similar to
> match_clauses_to_index.
Hi Amit
On Thu, Sep 28, 2017 at 1:44 PM, Amit Langote
wrote:
> On 2017/09/28 13:58, Dilip Kumar wrote:
>> I think the above logic is common between this patch and the runtime
>> pruning. I think we can make
>> a reusable function. Here we are preparing minkey and maxkey of Datum
>> because we can direc
On 2017/09/28 13:58, Dilip Kumar wrote:
> On Wed, Sep 27, 2017 at 6:52 AM, Amit Langote
> wrote:
>
> I was looking into the latest patch set, seems like we can reuse some
> more code between this path and runtime pruning[1]
>
> + foreach(lc1, matchedclauses[i])
> + {
> + Expr *clause = lfirst(
On Wed, Sep 27, 2017 at 6:52 AM, Amit Langote
wrote:
I was looking into the latest patch set, seems like we can reuse some
more code between this path and runtime pruning[1]
+ foreach(lc1, matchedclauses[i])
+ {
+ Expr *clause = lfirst(lc1);
+ Const *rightop = (Const *) get_rightop(clause);
+
On Wed, Sep 27, 2017 at 6:09 AM, Amit Langote wrote:
> On 2017/09/27 1:51, Robert Haas wrote:
> > On Tue, Sep 26, 2017 at 10:57 AM, Jesper Pedersen
> > wrote:
> >> One could advocate (*cough*) that the hash partition patch [1] should be
> >> merged first in order to find other instances of where
Hi Jesper.
Firstly, thanks for looking at the patch.
On 2017/09/26 22:00, Jesper Pedersen wrote:
> Hi Amit,
>
> On 09/15/2017 04:50 AM, Amit Langote wrote:
>> On 2017/09/15 11:16, Amit Langote wrote:
>>> I will post rebased patches later today, although I think the overall
>>> design of the patc
Hi David,
On 2017/09/27 6:04, David Rowley wrote:
> On 25 September 2017 at 23:04, Amit Langote
> wrote:
>> By the way, I'm now rebasing these patches on top of [1] and will try to
>> merge your refactoring patch in some appropriate way. Will post more
>> tomorrow.
>>
>> [1] https://git.postgres
On 2017/09/27 1:51, Robert Haas wrote:
> On Tue, Sep 26, 2017 at 10:57 AM, Jesper Pedersen
> wrote:
>> One could advocate (*cough*) that the hash partition patch [1] should be
>> merged first in order to find other instances of where other CommitFest
>> entries doesn't account for hash partitions
On 25 September 2017 at 23:04, Amit Langote
wrote:
> By the way, I'm now rebasing these patches on top of [1] and will try to
> merge your refactoring patch in some appropriate way. Will post more
> tomorrow.
>
> [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=9140cf826
Yeah,
On Tue, Sep 26, 2017 at 10:57 AM, Jesper Pedersen
wrote:
> One could advocate (*cough*) that the hash partition patch [1] should be
> merged first in order to find other instances of where other CommitFest
> entries doesn't account for hash partitions at the moment in their method
> signatures; Be
On 09/26/2017 10:33 AM, Robert Haas wrote:
On Tue, Sep 26, 2017 at 9:00 AM, Jesper Pedersen
wrote:
Could you share your thoughts on the usage of PartitionAppendInfo's
min_datum_idx / max_datum_idx ? Especially in relation to hash partitions.
This brings up something that I've kind of been thi
On Tue, Sep 26, 2017 at 9:00 AM, Jesper Pedersen
wrote:
> Could you share your thoughts on the usage of PartitionAppendInfo's
> min_datum_idx / max_datum_idx ? Especially in relation to hash partitions.
This brings up something that I've kind of been thinking about. There
are sort of four cases
Hi Amit,
On 09/15/2017 04:50 AM, Amit Langote wrote:
On 2017/09/15 11:16, Amit Langote wrote:
I will post rebased patches later today, although I think the overall
design of the patch on the planner side of things is not quite there yet.
Of course, your and others' feedback is greatly welcome.
On Tue, Sep 26, 2017 at 2:45 PM, Amit Langote
wrote:
> On 2017/09/25 20:21, Dilip Kumar wrote:
> I see. So, in the run-time pruning case, only the work of extracting
> bounding values is deferred to execution time. Matching clauses with the
> partition key still occurs during planning time. On
On 2017/09/25 20:21, Dilip Kumar wrote:
> On Mon, Sep 25, 2017 at 3:34 PM, Amit Langote
> wrote:
>
>> Thanks for looking at the patches and the comments.
>
>> It's not clear to me whether get_rel_partitions() itself, as it is, is
>> callable from outside the planner, because its signature contai
On Mon, Sep 25, 2017 at 3:34 PM, Amit Langote
wrote:
> Thanks for looking at the patches and the comments.
> It's not clear to me whether get_rel_partitions() itself, as it is, is
> callable from outside the planner, because its signature contains
> RelOptInfo. We have the RelOptInfo in the sig
Hi Dilip.
Thanks for looking at the patches and the comments.
On 2017/09/16 18:43, Dilip Kumar wrote:
> On Fri, Sep 15, 2017 at 2:20 PM, Amit Langote
> wrote:
>> On 2017/09/15 11:16, Amit Langote wrote:
>
> Thanks for the updated patch. I was going through the logic of
> get_rel_partitions in
I have done some refactoring of the code where I have moved the code
of getting the matching clause into the separate function so that it
can fetch the matching clause from any set of given restriction list.
It can be applied on top of 0002-WIP:
planner-side-changes-for-partition-pruning.patch
On
On Fri, Sep 15, 2017 at 2:20 PM, Amit Langote
wrote:
> On 2017/09/15 11:16, Amit Langote wrote:
Thanks for the updated patch. I was going through the logic of
get_rel_partitions in 0002 as almost similar functionality will be
required by runtime partition pruning on which Beena is working. The
On Sat, Sep 16, 2017 at 4:04 AM, Robert Haas wrote:
> On Fri, Sep 15, 2017 at 4:50 AM, Amit Langote
> wrote:
>> Rebased patches attached. Because Dilip complained earlier today about
>> clauses of the form (const op var) not causing partition-pruning, I've
>> added code to commute the clause whe
On Fri, Sep 15, 2017 at 4:50 AM, Amit Langote
wrote:
> Rebased patches attached. Because Dilip complained earlier today about
> clauses of the form (const op var) not causing partition-pruning, I've
> added code to commute the clause where it is required. Some other
> previously mentioned limita
On 2017/09/15 11:16, Amit Langote wrote:
> I will post rebased patches later today, although I think the overall
> design of the patch on the planner side of things is not quite there yet.
> Of course, your and others' feedback is greatly welcome.
Rebased patches attached. Because Dilip complaine
Hi Dilip,
Thanks for looking at the patch.
On 2017/09/15 13:43, Dilip Kumar wrote:
> On Wed, Sep 6, 2017 at 4:08 PM, Amit Langote
>> [PATCH 2/5] WIP: planner-side changes for partition-pruning
>>
>> This patch adds a stub get_partitions_for_keys in partition.c with a
>> suitable interface for the
On Wed, Sep 6, 2017 at 4:08 PM, Amit Langote
wrote:
> On 2017/09/04 10:10, Amit Langote wrote:
>> On 2017/09/02 2:52, Robert Haas wrote:
>
> [PATCH 2/5] WIP: planner-side changes for partition-pruning
>
> This patch adds a stub get_partitions_for_keys in partition.c with a
> suitable interface fo
On 2017/09/15 10:55, David Rowley wrote:
> On 21 August 2017 at 18:37, Amit Langote
> wrote:
>> I've been working on implementing a way to perform plan-time
>> partition-pruning that is hopefully faster than the current method of
>> using constraint exclusion to prune each of the potentially many
On 21 August 2017 at 18:37, Amit Langote wrote:
> I've been working on implementing a way to perform plan-time
> partition-pruning that is hopefully faster than the current method of
> using constraint exclusion to prune each of the potentially many
> partitions one-by-one. It's not fully cooked
On 2017/09/08 4:41, Robert Haas wrote:
> On Thu, Sep 7, 2017 at 7:16 AM, Amit Langote
> wrote:
>> There is a patch in the Ashutosh's posted series of patches, which does
>> more or less the same thing that this patch does. He included it in his
>> series of patches, because, IIUC, the main partit
On Thu, Sep 7, 2017 at 7:16 AM, Amit Langote
wrote:
> There is a patch in the Ashutosh's posted series of patches, which does
> more or less the same thing that this patch does. He included it in his
> series of patches, because, IIUC, the main partitionwise-join planning
> logic that one of the
Forgot to mention a couple of important points about the relation of some
of the patches here to the patches and discussion at the
partitionwise-join thread [1].
On 2017/09/06 19:38, Amit Langote wrote:
> [PATCH 1/5] Expand partitioned inheritance in a non-flattened manner
>
> This will allow us
On 2017/09/04 10:10, Amit Langote wrote:
> On 2017/09/02 2:52, Robert Haas wrote:
>> It strikes me that this patch set is doing two things but maybe in the
>> opposite order that I would have chosen to attack them. First,
>> there's getting partition pruning to use something other than
>> constrai
Thanks for the comments.
On 2017/09/02 2:52, Robert Haas wrote:
> On Thu, Aug 31, 2017 at 2:02 AM, Amit Langote
> wrote:
>> Attached is now also the set of patches that implement the actual
>> partition-pruning logic, viz. the last 3 patches (0004, 0005, and 0006) of
>> the attached.
>
> It stri
On Thu, Aug 31, 2017 at 2:02 AM, Amit Langote
wrote:
> Attached is now also the set of patches that implement the actual
> partition-pruning logic, viz. the last 3 patches (0004, 0005, and 0006) of
> the attached.
It strikes me that this patch set is doing two things but maybe in the
opposite ord
Hi Ashutosh,
Thanks for the comments and sorry that it took me a while to reply here.
On 2017/08/23 20:16, Ashutosh Bapat wrote:
> On Mon, Aug 21, 2017 at 12:07 PM, Amit Langote
> wrote:
>> I've been working on implementing a way to perform plan-time
>> partition-pruning that is hopefully faster
On Mon, Aug 21, 2017 at 12:07 PM, Amit Langote
wrote:
> I've been working on implementing a way to perform plan-time
> partition-pruning that is hopefully faster than the current method of
> using constraint exclusion to prune each of the potentially many
> partitions one-by-one. It's not fully c
74 matches
Mail list logo
