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

Am Dienstag, dem 12.11.2024 um 16:43 +0100 schrieb Bernd Helmle:
> 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....

Attached a new version of this patch. Note that i have combined the
previously separated patches for range types (patch to the backend) and
btree_gist into a single patch, since with the introduction of the
injection point "gist-sorted-build" this doesn't pass CI properly since
they're dependant now, which doesn't look very nice to me.

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

Workflow adjusted.

>
> > 2. Changes in contrib/btree_gist/btree_gist.c seem unnecessary.
> >
>
> Agreed. Will fix

Done

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

Done. Moved back to its original place.

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

Done, see below, too.

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

I've done it along your recommendation now. I followed the argument
that this way is far far less invasive from a code change perspective
and probably enough to check that sortsupport is used.

I've also spotted some oversights in the regression tests of the former
version of the patch, where i forgot to address the injection point
output in alternate expected files (char_1, text_1, ...) which are
created because of locale differences some time ago. This version also
addresses these.

Thanks,
Bernd