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’t 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 <[email protected]>
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
