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.lepik...@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.
