Обсуждение: Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

On Sat, Apr  8, 2017 at 11:36:13PM +0530, Pavan Deolasee wrote:
> Thank you all for the  reviews, feedback, tests, criticism. And apologies for
> keep pushing it till the last minute even though it was clear to me quite some
> time back the patch is not going to make it. But if I'd given up, it would have
> never received whatever little attention it got. The only thing that
> disappoints me is that the patch was held back on no strong technical grounds -
>  at least none were clear to me. There were concerns about on-disk changes etc,
> but most on-disk changes were known for 7 months now. Reminds me of HOT
> development, when it would not receive adequate feedback for quite many months,
> probably for very similar reasons - complex patch, changes on-disk format,
> risky, even though performance gains were quite substantial. I was much more
> hopeful this time because we have many more experts now as compared to then,
> but we probably have equally more amount of complex patches to review/commit.

I am sad to see WARM didn't make it into Postgres 10, but I agree
deferment was the right decision, as painful as that is.  We now have
something to look forward to in Postgres 11.  :-)

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

Hi,


On 2017-04-08 23:36:13 +0530, Pavan Deolasee wrote:
> On Wed, Apr 5, 2017 at 11:57 PM, Andres Freund <andres@anarazel.de> wrote:
> 
> > On 2017-04-05 09:36:47 -0400, Robert Haas wrote:
> > > By the way, the "Converting WARM chains back to HOT chains" section of
> > > README.WARM seems to be out of date.  Any chance you could update that
> > > to reflect the current state and thinking of the patch?
> >
> > I propose we move this patch to the next CF.  That shouldn't prevent you
> > working on it, although focusing on review of patches that still might
> > make it wouldn't hurt either.
> >
> >
> Thank you all for the  reviews, feedback, tests, criticism. And apologies
> for keep pushing it till the last minute even though it was clear to me
> quite some time back the patch is not going to make it.

What confuses me about that position is that people were advocating to
actually commit till literally hours before the CF closed.


> But if I'd given
> up, it would have never received whatever little attention it got. The only
> thing that disappoints me is that the patch was held back on no strong
> technical grounds -  at least none were clear to me. There were concerns
> about on-disk changes etc, but most on-disk changes were known for 7 months
> now. Reminds me of HOT development, when it would not receive adequate
> feedback for quite many months, probably for very similar reasons - complex
> patch, changes on-disk format, risky, even though performance gains were
> quite substantial. I was much more hopeful this time because we have many
> more experts now as compared to then, but we probably have equally more
> amount of complex patches to review/commit.

I don't think it's realistic to expect isolated in-depth review of
on-disk changes, when the rest of the patch isn't in a close-to-ready
shape. The likelihood that further work on the patch invalidates such
in-depth review is significant. It's not like only minor details changed
in the last few months.

I do agree that it's hard to get qualified reviewers on bigger patches.
But I think part of the reaction to that has to be active work on that
front: If your patch needs reviews by committers or other topical
experts, you need to explicitly reach out.  There's a lot of active
threads, and nobody has time to follow all of them in sufficient detail
to know that certain core parts of an actively developed patch are ready
for review.  Offer tit-for-tat reviews.  Announce that your patch is
ready, that you're only waiting for review.  Post a summary of open
questions...


> Finally, my apologies for not spending enough time reviewing other
> patches.  I know its critical, and I'll try to improve on that.

I do find it a more than a bit ironic to lament early lack of attention
to your patch, while also being aware of not having done much review.
This can only scale if everyone reviews each others patches, not if
there's a few individuals that have to review everyones patches.

Greetings,

Andres Freund

On Sat, Apr 8, 2017 at 2:06 PM, Pavan Deolasee <pavan.deolasee@gmail.com> wrote:
> Thank you all for the  reviews, feedback, tests, criticism. And apologies
> for keep pushing it till the last minute even though it was clear to me
> quite some time back the patch is not going to make it. But if I'd given up,
> it would have never received whatever little attention it got. The only
> thing that disappoints me is that the patch was held back on no strong
> technical grounds -  at least none were clear to me. There were concerns
> about on-disk changes etc, but most on-disk changes were known for 7 months
> now. Reminds me of HOT development, when it would not receive adequate
> feedback for quite many months, probably for very similar reasons - complex
> patch, changes on-disk format, risky, even though performance gains were
> quite substantial. I was much more hopeful this time because we have many
> more experts now as compared to then, but we probably have equally more
> amount of complex patches to review/commit.

Yes, and as Andres says, you don't help with those, and then you're
upset when your own patch doesn't get attention.  I think there are
two ways that this patch could have gotten the detailed and in-depth
review which it needs.  First, I would have been more than happy to
spend time on WARM in exchange for a comparable amount of your time
spent on parallel bitmap heap scan, or partition-wise join, or
partitioning, but that time was not forthcoming.  Second, there are
numerous senior reviewers at 2ndQuadrant who could have put time time
into this patch and didn't.  Yes, Alvaro did some review, but it was
not in a huge degree of depth and didn't arrive until quite late,
unless there was more to it than what was posted on the mailing list
which, as a reminder, is the place where review is supposed to take
place.

If people senior reviewers with whom you share an employer don't have
time to review your patch, and you aren't willing to trade review time
on other patches for a comparable amount of attention on your own,
then it shouldn't surprise you when people object to it being
committed.

If there is an intention to commit this patch soon after v11
development opens, then signs of serious in-depth review, and
responses to criticisms thus-far proffered, really ought to be in
evidence will in advance of that date.  It's slightly better to commit
an inadequately-reviewed patch at the beginning of the cycle than at
the end, but what's even better is thorough review, which I maintain
this patch hasn't really had yet.  Amit and others who have started to
dig into this patch a little bit found real problems pretty quickly
when they started digging.  Those problems should be addressed, and
review should continue (from whatever source) until no more problems
can be found.  Everyone here understands (if they've been paying
attention) that this patch has large benefits in sympathetic cases,
and everyone wants those benefits.  What nobody wants (I assume) is
regressions is unsympathetic cases, or data corruption.  The patch may
or may not have any data-corrupting bugs, but regressions have been
found and not addressed.  Yet, there's still talk of committing this
with as much haste as possible.  I do not think that is a responsible
way to do development.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Apr 10, 2017 at 04:34:50PM -0700, Andres Freund wrote:
> Hi,
> 
> 
> On 2017-04-08 23:36:13 +0530, Pavan Deolasee wrote:
> > On Wed, Apr 5, 2017 at 11:57 PM, Andres Freund <andres@anarazel.de> wrote:
> > 
> > > On 2017-04-05 09:36:47 -0400, Robert Haas wrote:
> > > > By the way, the "Converting WARM chains back to HOT chains" section of
> > > > README.WARM seems to be out of date.  Any chance you could update that
> > > > to reflect the current state and thinking of the patch?
> > >
> > > I propose we move this patch to the next CF.  That shouldn't prevent you
> > > working on it, although focusing on review of patches that still might
> > > make it wouldn't hurt either.
> > >
> > >
> > Thank you all for the  reviews, feedback, tests, criticism. And apologies
> > for keep pushing it till the last minute even though it was clear to me
> > quite some time back the patch is not going to make it.
> 
> What confuses me about that position is that people were advocating to
> actually commit till literally hours before the CF closed.

Yes, I was surprised by that too and have privately emailed people on
this topic.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Tue, Apr 11, 2017 at 10:50 PM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:
>
>
> On Tue, Apr 11, 2017 at 7:10 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>>
>>
>> Yes, and as Andres says, you don't help with those, and then you're
>> upset when your own patch doesn't get attention.
>
>
> I am not upset, I was obviously a bit disappointed which I think is a very
> natural emotion after spending weeks on it. I am not blaming any one
> individual (excluding myself) for that and neither the community at large
> for the outcome. And I've moved on. I know everyone is busy getting the
> release ready and I see no point discussing this endlessly. We have enough
> on our plates for next few weeks.
>
>>
>>
>>   Amit and others who have started to
>> dig into this patch a little bit found real problems pretty quickly
>> when they started digging.
>
>
> And I fixed them as quickly as humanly possible.
>

Yes, you have responded to them quickly, but I didn't get a chance to
re-verify all of those.  However, I think the main point Robert wants
to say is that somebody needs to dig the complete patch to see if
there is any kind of problems with it.

>>
>>   Those problems should be addressed, and
>> review should continue (from whatever source) until no more problems
>> can be found.
>
>
> Absolutely.
>
>>
>>  The patch may
>> or may not have any data-corrupting bugs, but regressions have been
>> found and not addressed.
>
>
> I don't know why you say that regressions are not addressed. Here are a few
> things I did to address the regressions/reviews/concerns, apart from fixing
> all the bugs discovered, but please let me know if there are things I've not
> addressed.
>
> 1. Improved the interesting attrs patch that Alvaro wrote to address the
> regression discovered in fetching more heap attributes. The patch that got
> committed in fact improved certain synthetic workloads over then master.
> 2. Based on Petr and your feedback, disabled WARM on toasted attributes to
> reduce overhead of fetching/decompressing the attributes.
> 3. Added code to avoid doing second index scan when the index does not
> contain any WARM pointers. This should address the situation Amit brought up
> where only one of the indexes receive WARM inserts.
> 4. Added code to kill wrong index pointers to do online cleanup.
> 5. Added code to set a CLEAR pointer to a WARM pointer when we know that the
> entire chain is WARM. This should address the workload Dilip ran and found
> regression (I don't think he got chance to confirm that)
>

Have you by any chance tried to reproduce it at your end?


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 11, 2017 at 1:20 PM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:
> I don't know why you say that regressions are not addressed. Here are a few
> things I did to address the regressions/reviews/concerns, apart from fixing
> all the bugs discovered, but please let me know if there are things I've not
> addressed.

I'm making statements based on my perception of the discussion on the
thread.  Perhaps you did some work which you either didn't mention or
I missed you mentioning it, but it sure didn't feel like all of the
things reported got addressed.

> 1. Improved the interesting attrs patch that Alvaro wrote to address the
> regression discovered in fetching more heap attributes. The patch that got
> committed in fact improved certain synthetic workloads over then master.

Yep, though it was not clear that all of the regressing cases were
actually addressed, at least not to me.

> 2. Based on Petr and your feedback, disabled WARM on toasted attributes to
> reduce overhead of fetching/decompressing the attributes.

But that's not necessarily the right fix, as per
http://postgr.es/m/CA+TgmoYUfxy1LseDzsw8uuuLUJHH0r8NCD-Up-HZMC1fYDPH3Q@mail.gmail.com
and subsequent discussion.  It's not clear to me from that discussion
that we've got to a place where the method used to identify whether a
WARM update happened during a scan is exactly identical to the method
used to decide whether to perform one in the first place.

> 3. Added code to avoid doing second index scan when the index does not
> contain any WARM pointers. This should address the situation Amit brought up
> where only one of the indexes receive WARM inserts
> 4. Added code to kill wrong index pointers to do online cleanup.

Good changes.

> 5. Added code to set a CLEAR pointer to a WARM pointer when we know that the
> entire chain is WARM. This should address the workload Dilip ran and found
> regression (I don't think he got chance to confirm that)

Which is clearly a thing that should happen before commit, and really,
you ought to be leading the effort to confirm that, not him.  It's
good for him to verify that your fix worked, but you should test it
first.

> 6. Enhanced stats collector to collect information about candidate WARM
> chains and added mechanism to control WARM cleanup at the heap as well as
> index level, based on configurable parameters. This gives user better
> control over the additional work that is required for WARM cleanup.

I haven't seen previous discussion of this; therefore I doubt whether
we have agreement on these parameters.

> 7. Added table level option to disable WARM if nothing else works.

-1 from me.

> 8. Added mechanism to disable WARM when more than 50% indexes are being
> updated. I ran some benchmarks with different percentage of indexes getting
> updated and thought this is a good threshold.

+1 from me.

> I may have missed something, but there is no intention to ignore known
> regressions/reviews. Of course, I don't think that every regression will be
> solvable, like if you run a CPU-bound workload, setting it up in a way such
> that you repeatedly exercise the area where WARM is doing additional work,
> without providing any benefit, may be you can still find regression. I am
> willing to fix them as long as they are fixable and we are comfortable with
> the additional code complexity. IMHO certain trade-offs are good, but I
> understand that not everybody will agree with my views and that's ok.

The point here is that we can't make intelligent decisions about
whether to commit this feature unless we know which situations get
better and which get worse and by how much.  I don't accept as a
general principle the idea that CPU-bound workloads don't matter.
Obviously, I/O-bound workloads matter too, but we can't throw
CPU-bound workloads under the bus.  Now, avoiding index bloat does
also save CPU, so it is easy to imagine that WARM could come out ahead
even if each update consumes slightly more CPU when actually updating,
so we might not actually regress.  If we do, I guess I'd want to know
why.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Apr 12, 2017 at 10:12 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> I may have missed something, but there is no intention to ignore known
>> regressions/reviews. Of course, I don't think that every regression will be
>> solvable, like if you run a CPU-bound workload, setting it up in a way such
>> that you repeatedly exercise the area where WARM is doing additional work,
>> without providing any benefit, may be you can still find regression. I am
>> willing to fix them as long as they are fixable and we are comfortable with
>> the additional code complexity. IMHO certain trade-offs are good, but I
>> understand that not everybody will agree with my views and that's ok.
>
> The point here is that we can't make intelligent decisions about
> whether to commit this feature unless we know which situations get
> better and which get worse and by how much.  I don't accept as a
> general principle the idea that CPU-bound workloads don't matter.
> Obviously, I/O-bound workloads matter too, but we can't throw
> CPU-bound workloads under the bus.  Now, avoiding index bloat does
> also save CPU, so it is easy to imagine that WARM could come out ahead
> even if each update consumes slightly more CPU when actually updating,
> so we might not actually regress.  If we do, I guess I'd want to know
> why.

I myself wonder if this CPU overhead is at all related to LP_DEAD
recycling during page splits. I have my suspicions that the recyling
has some relationship to locality, which leads me to want to
investigate how Claudio Freire's patch to consistently treat heap TID
as part of the B-Tree sort order could help, both in general, and for
WARM.

Bear in mind that the recycling has to happen with an exclusive buffer
lock held on a leaf page, which could hold up rather a lot of scans
that need to visit the same value even if it's on some other,
relatively removed leaf page.

This is just a theory.

-- 
Peter Geoghegan

VMware vCenter Server
https://www.vmware.com/