Re: [PATCH] Add sortsupport for range types and btree_gist

Am Montag, dem 11.11.2024 um 23:03 +0500 schrieb Andrey M. Borodin:
> Some nitpicking:
> 0. postgres % git apply ~/Downloads/v7.3-Add-GIST-sortsupport-*
> /Users/x4mmm/Downloads/v7.3-Add-GIST-sortsupport-btree-gist.patch:19:
> space before tab in indent.

I obviously shouldn't do patches after a long work day....


[...]

> 
> 1. I'm mostly seening patches formatted with `git patch-format`
> rather than diffs as patches...
> 

Will do.

> 2. Changes in contrib/btree_gist/btree_gist.c seem unnecessary.
> 

Agreed. Will fix

> 3. Why do you move "typedef struct int32key" to btree_gist.h, but do
> not need to move all other keys?
> 

Hmm, afair i did this back in an earlier stage of the patch and forgot
about it. It needs to be moved back to btree_int4.c, will fix.

> 4. These ifdedfs are not needed, just do INJECTION_POINT()
> #ifdef USE_INJECTION_POINTS
> INJECTION_POINT("btree-gist-sorted-build");
> #endif
> 

I never worked with injection points before, i copied that pattern from
the docs somewhere, where those #ifdef's are used. Will fix.

> Also, there are 61 occurance of this code. Why not just 1 in
> gist_indexsortbuild() ?
> 
> 

Right, but after thinking about this i'd even go further: naming the
injection points for the sortsupport according to their datatype
they're called on. That would connect the regression test with the
specific datatype/sortsupport function and this would make sure the
correct sortsupport function for the specific test case is called.
Though that means that each test needs to attach/detach the injection
point itself...

What do you think?

Thanks,
	Bernd