Re: [DOCS] HOT - correct claim about indexes not referencing old line pointers - Mailing list pgsql-hackers

On Wed, Mar 20, 2024 at 2:15 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Mar 14, 2024 at 9:07 PM James Coleman <jtc331@gmail.com> wrote:
> > Obviously I have reasons for the other changes I made: for example,
> > "no longer visible" improves the correctness, since being an old
> > version isn't sufficient. I removed the "In summary" sentence because
> > it simply doesn't follow from the prose before it. That sentence
> > simply restates information already appearing earlier in almost as
> > simple a form, so it's redundant. But more importantly it's just not
> > actually a summary of the text before it, so removing it improves the
> > documentation.
> >
> > I can explain my reasoning further if desired, but I fear it would
> > simply frustrate you further, so I'll stop here.
> >
> > If the goal here is the most minimal patch possible, then please
> > commit what you proposed. I am interested in improving the document
> > further, but I don't know how to do that easily if the requirement is
> > effectively "must only change one specific detail at a time". So, that
> > leaves me feeling a bit frustrated also.
>
> I don't think the goal is to have the most minimal patch possible, but
> at the same time, I'm responsible for what I commit. If I commit a
> patch that changes something and somebody, especially another
> committer, writes an email and says "hey, why the heck did you change
> this, you dummy?" then I need to be able to answer that question, and
> saying "uh, well, James Coleman had it in his patch and he didn't
> explain why it was there and I didn't know either but I just kind of
> committed it anyway" is not where I want to be. Almost without
> exception, it's not the patch author who gets asked why they included
> a certain thing in the patch; it's the committer who gets asked why it
> got committed that way -- and at least as I see it, if there's not a
> good answer to that question, it's the committer who gets judged, not
> the patch author. In some cases, such questions and judgements arrive
> without warning YEARS after the commit.
>
> So, to protect myself against questions from other hackers that end,
> at least implicitly, in "you dummy," I try to make sure there's an
> adequate paper trail for everything I commit. Between the commit
> message, the comments, and the email discussion, it needs to be
> crystal clear why something was thought to be a good idea at the time.
> Hopefully, it will be clear enough that I never even get a question,
> because the reader will be able to understand ON THEIR OWN why
> something was done and either go "oh, that make sense" or "well, I get
> why they did that, but I disagree because $REASON" ... but if that
> doesn't quite work out, then I hope that at the very least the paper
> trail will be good enough that I can reconstruct the reasoning if
> needed. For a recent example of a case where I clearly failed do a
> good enough job to keep someone from asking a "you dummy" question,
> see the http://postgr.es/m/6030bdec-0de1-4da2-b0b3-335007eae97f@iki.fi
> and in particular the paragraph that starts with "However".
>
> Therefore, if I realize when reading the patch that it contains
> odd-looking changes which I can't relate to the stated goal, it's
> getting bounced, pretty much 100% of the time. If it's a small patch
> and I really care about it and it's a new submitter who doesn't know
> any better, I might choose to remove those changes myself and commit
> the rest; and if I like those changes for other reasons, I might
> choose to commit them separately. In any other situation, I'm just
> going to tell the submitter that they need to take out the unrelated
> changes and move on to the next email thread.
>
> In this particular situation, I see that this whole discussion is
> slightly moot by virtue of 7a9328e8e40534fb4de41b4ac152e3c6989d84e7
> (Jeff Davis, 2024-03-05) which removed the "In summary, heap-only
> tuple updates can only be created if columns used by indexes are not
> updated" sentence that you also removed. But there is an important
> difference between that patch and yours, at least IMHO. You argue that
> the sentence in question didn't flow from what precedes it, but I
> don't agree with that as a rationale; I think that sentence flows
> perfectly well from what precedes it and is a generally adequate
> summary of the point of the section. That we disagree on the merits of
> that sentence is fine; there's no hope that we're all going to agree
> on everything. What makes me frustrated is that you didn't provide
> that rationale, which greatly complicates the whole discussion,
> because now I have to spend time getting you to either take it out or
> provide your justification before we can even reach the point of
> arguing about the substance.
>
> At least in my judgment, the patch Jeff committed doesn't have that
> problem, because the rationale that he gives in the commit message
> (partly by reference to another commit) is that heap-only tuples now
> can, in circumstances, be created even if columns used by indexes ARE
> updated. Based on that justification, it seems really clear to me that
> it was right to do SOMETHING to the sentence in question. Whether
> removing the sentence was better than some other alternative is
> debatable, and I might well have done something different, but between
> reading the commit message and reading the diff, it's 100%
> understandable to me what his reasoning was for every single byte that
> is in that diff. So if I love what he committed, cool! And if I hate
> what he committed, I'm well-placed to argue, because I know what I'm
> arguing against.
>
> Also, if I do feel like arguing, the fact that I know his reasoning
> will also make it a heck of a lot easier to come up with a proposal
> that meets my goals without undermining his. I can see, for example,
> that I can't simply propose to put that sentence back the way that it
> was; based on his reason for removing it, he obviously isn't going to
> like that. But if, say, my goal were to have some kind of a summary
> sentence there because I just thought that was really helpful, I could
> write a patch to insert a new summary sentence there and then say
> "hey, Jeff, you took this summary sentence out because it's not
> accurate, but here's a new one which I think is accurate and I'd like
> to add it back." Maybe he'd like that, and maybe he wouldn't, but it
> would have a shot, because it would take his reasoning into account,
> which I can do, because I know what it was. I want people who read the
> patches that I commit to have the same opportunities.

I'm happy to provide the rationale; my apologies for not providing it
originally.

I didn't realize that was the root of the frustration: I'd read your
feedback ("[my proposed change[ is directly responsive to your
original complaint...[your] version seems to contain a number of
incidental changes that are unrelated to that") as being aimed at
something like "a patch can only address the original specific
complaint".

I'll try to be more specific in my reasoning when e.g. I think a piece
of documentation reads better with a few additional changes. If I
forget to do that I'd also appreciate your asking up front for the
rationale so that I have a chance to clarify that without
misunderstanding.

Regards,
James Coleman