Date: Fri, 23 Feb 2024 21:12:05 -0800
From: Noah Misch <noah@leadboat.com>
To: Richard Guo <guofenglinux@gmail.com>,
	Andrei Lepikhov <a.lepikhov@postgrespro.ru>,
	Alexander Korotkov <aekorotkov@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>,
	"Gregory Stark (as CFM)" <stark.cfm@gmail.com>,
	=?utf-8?Q?Micha=C5=82_K=C5=82eczek?= <michal@kleczek.org>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: Removing unneeded self joins
Message-ID: <20240224051205.db@rfd.leadboat.com>
References: <4788f781-31bd-9796-d7d6-588a751c8787@gmail.com>
 <CAPpHfduYKqbWWT=YuBJyoExrwsJSgjUuJN1Gsr2YZMG=nh_ctQ@mail.gmail.com>
 <CAPpHfdvu+wyazT+ujJbJ5rKZrHQoG+FCVeWENA4pyf_A1W1WAA@mail.gmail.com>
 <56ee4520-e9d1-d519-54fe-c8bff880ce9b@gmail.com>
 <CAPpHfdsJ=MZNFw0L2T_k-shOk+4dPNx9j5RZzicy3NW+yiiQVg@mail.gmail.com>
 <CAPpHfdtB0A1Rz8k5yPMCM6ngLt2JM4NSaG8rF0=wT-txdcEf3g@mail.gmail.com>
 <5063f378-ca3c-405e-b1e8-1927ea9052e8@postgrespro.ru>
 <CAMbWs4-=PO6Mm9gNnySbx0VHyXjgnnYYwbN9dth=TLQweZ-M+g@mail.gmail.com>
 <d6d227ff-0284-412c-aecf-603ad895873e@postgrespro.ru>
 <CAPpHfdt57GeLokML7pYaecejV-VfHVDpVqbx-hTVswWmQ6nMTQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAPpHfdt57GeLokML7pYaecejV-VfHVDpVqbx-hTVswWmQ6nMTQ@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20240224051205.db%40rfd.leadboat.com>
Precedence: bulk

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.