Date: Mon, 17 Jan 2022 15:39:33 +0800
From: Julien Rouhaud <rjuju123@gmail.com>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Thomas <thomasmannhart97@gmail.com>, daniel@yesql.se,
	Jaime Casanova <jcasanov@systemguards.com.ec>,
	pgsql <pgsql@j-davis.com>, pgsql-hackers@lists.postgresql.org,
	dignoes@inf.unibz.it, boehlen@ifi.uzh.ch, p.moser@noi.bz.it,
	gamper@inf.unibz.it, Thomas Mannhart <thomas_m@hotmail.ch>
Subject: Re: Patch: Range Merge Join
Message-ID: <20220117073933.m7ef2hwaubpcd75v@jrouhaud>
References: <CAMWfgiAsJgVrkbrsv740Y2=2duO4rVYRhaD08EhFqBuJFmBH1A@mail.gmail.com>
 <CAApHDvoyAAkJjZ=DxyOMDhSNDg3Fo799OAF6c8gzfLWBMNaK0Q@mail.gmail.com>
 <2a14f6f50f103773c843164ba7572fa0a2e27d9f.camel@j-davis.com>
 <20211004212754.GA30366@ahch-to>
 <20211005003034.GA30929@ahch-to>
 <CAMWfgiDDvHqN2STsCyKj9oyBJ1yCQdDOp_6Jc0xupuJ8fn9xtQ@mail.gmail.com>
 <ABDD974C-E280-4C08-B696-EE58AB418624@yesql.se>
 <CAMWfgiBRETALLvhk9P3dCsii8cL1XyYHv87=MH4OtnT5Y+o9Ng@mail.gmail.com>
 <1dbe10e3-2264-d6ad-bd1f-6f98cf474818@enterprisedb.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1dbe10e3-2264-d6ad-bd1f-6f98cf474818@enterprisedb.com>
Archived-At: <https://www.postgresql.org/message-id/20220117073933.m7ef2hwaubpcd75v%40jrouhaud>
Precedence: bulk

Hi,

On Wed, Nov 17, 2021 at 11:28:43PM +0100, Tomas Vondra wrote:
> On 11/17/21 15:45, Thomas wrote:
> > Thank you for the feedback and sorry for the oversight. I fixed the bug
> > and attached a new version of the patch.
> > 
> > Kind Regards, Thomas
> > 
> > Am Mi., 17. Nov. 2021 um 15:03 Uhr schrieb Daniel Gustafsson
> > <daniel@yesql.se <mailto:daniel@yesql.se>>:
> > 
> >     This patch fails to compile due to an incorrect function name in an
> >     assertion:
> > 
> >        nodeMergejoin.c:297:9: warning: implicit declaration of function
> >     'list_legth' is invalid in C99 [-Wimplicit-function-declaration]
> >        Assert(list_legth(node->rangeclause) < 3);
> > 
> 
> That still doesn't compile with asserts, because MJCreateRangeData has
> 
>     Assert(list_length(node->rangeclause) < 3);
> 
> but there's no 'node' variable :-/
> 
> 
> I took a brief look at the patch, and I think there are two main issues
> preventing it from moving forward.
> 
> 1) no tests
> 
> 2) lack of comments
> 
> 3) I'm not quite sure I like "Range Merge Join" to be honest. It's still a
> "Merge Join" pretty much. What about ditching the "Range"? There'll still be
> "Range Cond" key, which should be good enough I think.
> 
> 4) Some minor whitespace issues (tabs vs. spaces). See 0002.

It's been 2 months since Tomas posted that review.

Thomas, do you plan to work on that patch during this commitfest?