Re: Patch: Write Amplification Reduction Method (WARM)

On Sat, Apr 8, 2017 at 2:06 PM, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)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