Feedback-ID: ie0a040ee:Fastmail
Message-ID: <a35eaf0f-b19c-7797-e296-7b7e2adc473e@eisentraut.org>
Date: Tue, 22 Aug 2023 08:43:08 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.13.0
Subject: Re: pgsql: Improve BRIN minmax-multi opclass test coverage
Content-Language: en-US
To: Tomas Vondra <tomas.vondra@postgresql.org>,
 pgsql-committers@lists.postgresql.org
References: <E1qFsXm-001Uqd-Tv@gemulon.postgresql.org>
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <E1qFsXm-001Uqd-Tv@gemulon.postgresql.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/a35eaf0f-b19c-7797-e296-7b7e2adc473e%40eisentraut.org>
Precedence: bulk

These new tests introduced a few new calls of the md5() function.  This 
should be avoided (see commit 208bf364a9).  You can replace these easily 
with the fipshash() function instead, but I think you then also need to 
change some of the constants in the tests.  Could you look at this again?


On 02.07.23 10:34, Tomas Vondra wrote:
> Improve BRIN minmax-multi opclass test coverage
> 
> Per the code coverage report, the existing regression tests did not
> exercice some a couple important BRIN minmax-multi code paths.
> 
> - The tests focused on testing planning with a range of scan key
>    strategies, but not the execution. Fixed by adding queries that
>    actually test query execution for both equality and inequality.
> 
> - All tests created indexes after inserting data, but this only
>    exercises the CREATE INDEX strategy that sees all values at once, not
>    incremental summary updates. The new tests flip the order and create
>    the index before adding data.
> 
> - The assert check(s) validating correctness of expanded ranges were
>    present only in the "union" code path, which is not covered by
>    regression tests at all (as it requires concurrency etc.). Fixed by
>    adding the asserts to a couple more places.
> 
> Reviewed-by: Heikki Linnakangas
> Discussion: https://postgr.es/m/57020b2e-d9c9-9bc7-4892-b36d9bb07563%40enterprisedb.com
> 
> Branch
> ------
> master
> 
> Details
> -------
> https://git.postgresql.org/pg/commitdiff/0457109344b46f481f9bf09b85674695ba57c8e4
> 
> Modified Files
> --------------
> src/backend/access/brin/brin_minmax_multi.c |   9 +
> src/test/regress/expected/brin_multi.out    | 357 ++++++++++++++++++++++++++++
> src/test/regress/sql/brin_multi.sql         | 165 +++++++++++++
> 3 files changed, 531 insertions(+)
>