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

Hi Michael,

Am Freitag, dem 29.11.2024 um 13:42 +0900 schrieb Michael Paquier:

[...]

> Any module that requires the module injection_points to be installed
> can do a few things, none of them are done this way in this patch:
> - Add EXTRA_INSTALL=src/test/modules/injection_points.
> - You could make a test able to run installcheck, but you should use
> an extra query that checks if the module is available for
> installation
> by querying pg_available_extensions and use two possible output
> files:
> one for the case where the module is *not* installed and one for the
> case where the module is installed.  A simpler way would be to block
> installcheck, or add SQL tests in src/test/modules/injection_points.
> Both options don't seem adapted to me here as they impact the
> portability of existing tests.
>

I don't like this. This smells like we use the wrong tool. Andrey had
the idea to use them because it looked as a compelling idea to check
whether sortsupport is used in the code path or not.

Maybe we should just use a specific DEBUG message and make sure it's
handled by the tests accordingly.

> As a whole, I'm very dubious about the need for injection points at
> all here.  The sortsupport property claimed for this patch tells that
> this results in smaller index sizes, but the tests don't really check
> that: they just make sure that sortsupport routine paths are taken.
> What this should test is not the path taken, but how the new code
> affects the index data generated.  Perhaps pageinspect could be
> something to use to show the difference in contents, not sure though.
>

No, this isn't the goal of sortsupport for btree_gist. It was a side
effect before PG15 that it yielded a better index quality, but the
primary goal is and was index creation speed. With sortsupport we're
much much faster that the traditional buffered method.

Originally this patch was pushed by a customer who had complains about
the long build times of btree_gist indexes used by exclusion
constraints they're heavily relying on. See a benchmark i've posted in
this thread here with test data:

https://www.postgresql.org/message-id/2f2e88bf1d44d06c1baf27bbf3db880f42a5cb87.camel@oopsware.de

> The number of tests added to contrib/btree_gist/Makefile is not
> acceptable for a patch of this size, leading to a large bloat.  And
> that's harder to maintain in the long-term.

Yes, it is not really nice, but i have no better idea atm on how to
test both code paths equally without repeating the tests again. Index
creation must yield the same results than buffered index build
strategy. Originally i just added the tests to the same test file, but
then decided to split them up to make it simpler to review and
maintain.

I'm open for better ideas.

Thanks for your comments

Bernd