Re: Reviewing freeze map code

On Mon, Jun 6, 2016 at 4:06 PM, Andres Freund <andres@anarazel.de> wrote:
>> I do appreciate you reviewing this code, very much, and genuinely, and
>> it would be great if more people wanted to review it.
>
>> But this kind of reads like you think that I'm being a jerk, which I'm
>> trying pretty hard not to be
>
> I don't think you're a jerk. But I am loosing a good bit of my patience
> here. I've posted these issues a month ago, and for a long while the
> only thing that happened was bikeshedding about the name of something
> that wasn't even decided to happen yet (obviously said bikeshedding
> isn't your fault).

No, the bikeshedding is not my fault.

As for the timing, you posted your first comments exactly a week
before beta1 when I was still busy addressing issues that were
reported before you reported yours, and I did not think it was
realistic to get them addressed in the time available.  If you'd sent
them two weeks sooner, I would probably have done so.  Now, it's been
four weeks since beta1 wrapped, one of which was PGCon.  As far as I
understand at this point in time, your review identified exactly zero
potential data loss bugs.  (We thought there was one, but it looks
like there isn't.)  All of the non-critical defects you identified
have now been fixed, apart from the lack of a better testing tool.
And since there is ongoing discussion (call it bikeshedding if you
want) about what would actually help in that area, I really don't feel
like anything very awful is happening here.

I really don't understand how you can not weigh in on the original
thread leading up to my mid-March commits and say "hey, this needs a
better testing tool", and then when you finally get around to
reviewing it in May, I'm supposed to drop everything and write one
immediately.  Why do you get two months from the time of commit to
weigh in but I get no time to respond?  For my part, I thought I *had*
written a testing tool - that's what pg_visibility is and that's what
I used to test the feature before committing it.  Now, you think
that's not good enough, and I respect your opinion, but it's not as if
you said this back when this was being committed.  Or at least if you
did, I don't remember it.

>> and like you have the right to tell assign me arbitrary work, which I
>> think you don't.
>
> It's not like adding a parameter for this would be a lot of work,
> there's even a patch out there.  I'm getting impatient because I feel
> the issue of this critical feature not being testable is getting ignored
> and/or played down.  And then sidetracked into a general "let's add a
> database consistency checker" type discussion. Which we need, but won't
> get in 9.6.

I know there's a patch.  Both Tom and I are skeptical about whether it
adds value, and I really don't think you've spelled out in as much
detail why you think it will help as I have why I think it won't.
Initially, I was like "ok, sure, we should have that", but the more I
thought about it (another advantage of time passing: you can think
about things more) the less convinced I was that it did anything
useful.  I don't think that's very unreasonable.  The importance of
the feature is exactly why we *should* think carefully about what is
best here and not just do the first thing that pops into our head.

> If you say: "I agree with the feature in principle, but I don't want to
> spend time to review/commit it." - ok, that's fair enough. But at the
> moment that isn't what I'm reading between the lines.

No, what I'm saying is "I'm not confident that this feature adds
value, and I'm afraid that by adding it we are making ourselves feel
better without solving any real problem".  I'm also saying "let's try
to agree on what problems we need to solve first and then decide on
the solutions".

>> If you want to have a
>> reasonable conversation about what the options are for making this
>> better, great.
>
> Yes, I want that.

Great.

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