Re: Patch: Range Merge Join

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

There's not a *single* regression test exercising the new code, so even 
after adding Assert(false) to MJCreateRangeData() tests pass just fine. 
Clearly, that needs to change.

2) lack of comments

The patch adds a bunch of functions, but it does not really explain what 
the functions do (unlike the various surrounding functions). Even if I 
can work out what the functions do, it's much harder to determine what 
the "contract" is (i.e. what assumptions the function do and what is 
guaranteed).

Similarly, the patch modifies/reworks large blocks of executor code, 
without updating the comments describing what the block does.

See 0002 for various places that I think are missing comments.


Aside from that, I have a couple minor 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.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company