index-only plans

[Till Westmann <ti...@apache.org>]

Hi,

we still have the big change on index-only plans outstanding. I think that
it would be good to have that feature. However, at it\u2019s current size (+45K
lines, -15K lines) it is very (!) difficult to review. So I think that one
approach to get there would be to break it down into smaller more achievable
steps.
I\u2019ve added a few comments to the review with thoughts I had to do that.
What do you think?
Is that a good approach? Is it feasible?
Are there other ways?

Thanks for your thoughts,
Till

Re: index-only plans

[Taewoo Kim <wa...@gmail.com>]

@Chen, @Till: Thanks. I will try.

Best,
Taewoo

On Thu, Aug 11, 2016 at 10:55 PM, Chen Li <ch...@gmail.com> wrote:

> I am always a big fan of separating a big merge into multiple small
> changes.  It will be good to do this "partitioning."
>
> Chen
>
> On Thu, Aug 11, 2016 at 2:46 PM, Taewoo Kim <wa...@gmail.com> wrote:
>
> > Thanks Till for reviewing this giant patch set.
> >
> > At this moment, what I can try to do is removing all necessary test cases
> > and changes that are related to full-text search preparation (changing
> the
> > function name of "contains" to "string-contains") since I thought this
> > index-only plan branch could be merged first.
> >
> > I tried to separate logical LIMIT push-down to the index search and
> > index-only plan. But, it turns out that it was hard. Other than this, all
> > changes are related to index-only plan part (most of them are
> accessMethod
> > related.) In addition, Young-Seok already had one round.
> >
> >
> > Best,
> > Taewoo
> >
> > On Thu, Aug 11, 2016 at 2:43 PM, Till Westmann <ti...@apache.org> wrote:
> >
> > > Hi,
> > >
> > > we still have the big change on index-only plans outstanding. I think
> > that
> > > it would be good to have that feature. However, at it’s current size
> > (+45K
> > > lines, -15K lines) it is very (!) difficult to review. So I think that
> > one
> > > approach to get there would be to break it down into smaller more
> > > achievable
> > > steps.
> > > I’ve added a few comments to the review with thoughts I had to do that.
> > > What do you think?
> > > Is that a good approach? Is it feasible?
> > > Are there other ways?
> > >
> > > Thanks for your thoughts,
> > > Till
> > >
> >
>

Re: index-only plans

[Chen Li <ch...@gmail.com>]

I am always a big fan of separating a big merge into multiple small
changes.  It will be good to do this "partitioning."

Chen

On Thu, Aug 11, 2016 at 2:46 PM, Taewoo Kim <wa...@gmail.com> wrote:

> Thanks Till for reviewing this giant patch set.
>
> At this moment, what I can try to do is removing all necessary test cases
> and changes that are related to full-text search preparation (changing the
> function name of "contains" to "string-contains") since I thought this
> index-only plan branch could be merged first.
>
> I tried to separate logical LIMIT push-down to the index search and
> index-only plan. But, it turns out that it was hard. Other than this, all
> changes are related to index-only plan part (most of them are accessMethod
> related.) In addition, Young-Seok already had one round.
>
>
> Best,
> Taewoo
>
> On Thu, Aug 11, 2016 at 2:43 PM, Till Westmann <ti...@apache.org> wrote:
>
> > Hi,
> >
> > we still have the big change on index-only plans outstanding. I think
> that
> > it would be good to have that feature. However, at it’s current size
> (+45K
> > lines, -15K lines) it is very (!) difficult to review. So I think that
> one
> > approach to get there would be to break it down into smaller more
> > achievable
> > steps.
> > I’ve added a few comments to the review with thoughts I had to do that.
> > What do you think?
> > Is that a good approach? Is it feasible?
> > Are there other ways?
> >
> > Thanks for your thoughts,
> > Till
> >
>

Re: index-only plans

[Till Westmann <ti...@apache.org>]

Hi Taewoo,

I think that pulling the function rename out would be a great step 
forward!
Please do this.

Cheers,
Till

P.S. Please also look at my other comments. E.g. there are whitespace 
and
formatting changes that increase the number of changes files and lines 
that
each reviewer needs to look at. While these don\u2019t change the behavior, 
some
reviewers (I am one of those) see those multiple times as they go back 
and
forth though the change and need to convince themselves multiple times 
that
they are benign :)

On 11 Aug 2016, at 14:46, Taewoo Kim wrote:

> Thanks Till for reviewing this giant patch set.
>
> At this moment, what I can try to do is removing all necessary test 
> cases
> and changes that are related to full-text search preparation (changing 
> the
> function name of "contains" to "string-contains") since I thought this
> index-only plan branch could be merged first.
>
> I tried to separate logical LIMIT push-down to the index search and
> index-only plan. But, it turns out that it was hard. Other than this, 
> all
> changes are related to index-only plan part (most of them are 
> accessMethod
> related.) In addition, Young-Seok already had one round.
>
>
> Best,
> Taewoo
>
> On Thu, Aug 11, 2016 at 2:43 PM, Till Westmann <ti...@apache.org> 
> wrote:
>
>> Hi,
>>
>> we still have the big change on index-only plans outstanding. I think 
>> that
>> it would be good to have that feature. However, at it\u2019s current 
>> size (+45K
>> lines, -15K lines) it is very (!) difficult to review. So I think 
>> that one
>> approach to get there would be to break it down into smaller more
>> achievable
>> steps.
>> I\u2019ve added a few comments to the review with thoughts I had to do 
>> that.
>> What do you think?
>> Is that a good approach? Is it feasible?
>> Are there other ways?
>>
>> Thanks for your thoughts,
>> Till
>>

Re: index-only plans

[Taewoo Kim <wa...@gmail.com>]

Thanks Till for reviewing this giant patch set.

At this moment, what I can try to do is removing all necessary test cases
and changes that are related to full-text search preparation (changing the
function name of "contains" to "string-contains") since I thought this
index-only plan branch could be merged first.

I tried to separate logical LIMIT push-down to the index search and
index-only plan. But, it turns out that it was hard. Other than this, all
changes are related to index-only plan part (most of them are accessMethod
related.) In addition, Young-Seok already had one round.


Best,
Taewoo

On Thu, Aug 11, 2016 at 2:43 PM, Till Westmann <ti...@apache.org> wrote:

> Hi,
>
> we still have the big change on index-only plans outstanding. I think that
> it would be good to have that feature. However, at it’s current size (+45K
> lines, -15K lines) it is very (!) difficult to review. So I think that one
> approach to get there would be to break it down into smaller more
> achievable
> steps.
> I’ve added a few comments to the review with thoughts I had to do that.
> What do you think?
> Is that a good approach? Is it feasible?
> Are there other ways?
>
> Thanks for your thoughts,
> Till
>