Feedback-ID: ie3de48e3:Fastmail
Date: Sun, 6 Jul 2025 21:49:54 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: Arseniy Mukhin <arseniy.mukhin.dev@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: amcheck support for BRIN indexes
Message-ID: <202507061949.6tjgborub3kd@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAE7r3MJFTso-uT2dTGg5rdvEHgyQQAu7YFR5sDUZLAtnC2ThCw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202507061949.6tjgborub3kd%40alvherre.pgsql>
Precedence: bulk

On 2025-Jul-06, Arseniy Mukhin wrote:

> Sorry, forget to run a full test run with the new patch version. Some
> tests were unhappy with the new unknown support function. Here the new
> version with the fix.

Hello, I think this patch is probably a good idea.  I don't think it
makes sense to introduce a bunch of code in 0003 only to rewrite it
completely in 0005.  I would ask that you re-split your WITHIN_RANGE
(0004) to appear before the amcheck code, and then write the amcheck
code using that new functionality.

>  /*
>   * Return a tuple descriptor used for on-disk storage of BRIN tuples.
>   */
> -static TupleDesc
> +TupleDesc
>  brtuple_disk_tupdesc(BrinDesc *brdesc)

I think we should give this function a better name if it's going to be
exported.  How about brin_tuple_tupdesc?  (in brin_tuple.h we
seem to distinguish "brin tuples" which are the stored ones, from "brin
mem tuples" which are the ones to be used in memory.)

I didn't read the other patches.

Thanks

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"I'm impressed how quickly you are fixing this obscure issue. I came from 
MS SQL and it would be hard for me to put into words how much of a better job
you all are doing on [PostgreSQL]."
 Steve Midgley, http://archives.postgresql.org/pgsql-sql/2008-08/msg00000.php