Re: Removing unneeded self joins

Hi, Noah!

On Sat, Feb 24, 2024 at 7:12 AM Noah Misch <noah@leadboat.com> wrote:
> On Sat, Feb 24, 2024 at 12:36:59AM +0200, Alexander Korotkov wrote:
> > On Thu, Feb 22, 2024 at 10:51 AM Andrei Lepikhov <a.lepikhov@postgrespro.ru> wrote:
> > > On 21/2/2024 14:26, Richard Guo wrote:
> > > > I think the right fix for these issues is to introduce a new element
> > > > 'sublevels_up' in ReplaceVarnoContext, and enhance replace_varno_walker
> > > > to 1) recurse into subselects with sublevels_up increased, and 2)
> > > > perform the replacement only when varlevelsup is equal to sublevels_up.
> > > This code looks good. No idea how we have lost it before.
> >
> > Thanks to Richard for the patch and to Andrei for review.  I also find
> > code looking good.  Pushed with minor edits from me.
>
> I feel this, commit 466979e, misses a few of our project standards:
>
> - The patch makes many non-whitespace changes to existing test queries.  This
>   makes it hard to review the consequences of the non-test part of the patch.
>   Did you minimize such edits?  Of course, not every such edit is avoidable.
>
> - The commit message doesn't convey whether this is refactoring or is a bug
>   fix.  This makes it hard to write release notes, among other things.  From
>   this mailing list thread, it gather it's a bug fix in 489072ab7a, hence
>   v17-specific.  The commit message for 489072ab7a is also silent about that
>   commit's status as refactoring or as a bug fix.
>
> - Normally, I could answer the previous question by reading the test case
>   diffs.  However, in addition to the first point about non-whitespace
>   changes, the first three join.sql patch hunks just change whitespace.
>   Worse, since they move line breaks, "git diff -w" doesn't filter them out.
>
> To what extent are those community standards vs. points of individual
> committer preference?  Please tell me where I'm wrong here.

I agree that commit 466979e is my individual committer failure.  I
should have written a better, more clear commit message and separate
tests refactoring from the bug fix.

I'm not so sure about 489072ab7a (except it provides a wrong fix).  It
has a "Reported-by:" field meaning it's a problem reported by a
particular person.  The "Discussion:" points directly to the reported
test case.  And commit contains the relevant test case.  The commit
message could be more wordy though.

------
Regards,
Alexander Korotkov