Обсуждение: Add IS_INDEX macro to brin and gist index
Hi, hackers, While working on pageinspect [0], I noticed that brin_page_items() and gist_page_items() only checked the access method (IS_BRIN/IS_GIST) but did not verify that the passed relation is actually an index relation. To make the check more robust and consistent with other pageinspect index functions (like btreefuncs.c, hashfuncs.c, etc.), the attached patch: 1. Defines a local helper macro IS_INDEX(r) in both brinfuncs.c and gistfuncs.c. 2. Updates the error check to require both: the relation must be an index and use the expected access method. The change is very small, low-risk, and only affects two functions in contrib/pageinspect. [0]: https://www.postgresql.org/message-id/CALdSSPiN13n7feQcY0WCmq8jzxjwqhNrt1E=g=g6aZANyE_OoQ@mail.gmail.com -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Вложения
On 1/14/26 2:56 AM, Japin Li wrote: > > Hi, hackers, > > While working on pageinspect [0], I noticed that brin_page_items() and > gist_page_items() only checked the access method (IS_BRIN/IS_GIST) but did > not verify that the passed relation is actually an index relation. > > To make the check more robust and consistent with other pageinspect index > functions (like btreefuncs.c, hashfuncs.c, etc.), the attached patch: > > 1. Defines a local helper macro IS_INDEX(r) in both brinfuncs.c and gistfuncs.c. > > 2. Updates the error check to require both: the relation must be an index and > use the expected access method. > > The change is very small, low-risk, and only affects two functions in > contrib/pageinspect. Since the two functions you touch are gist_page_items() and brin_page_items() is there actually any harm from being able to use the index definition from a partitioned when parsing the page? Seems unnecessary to prevent people from doing so unless I am missing something. It is not like we have any way to prevent the wrong index from being used when parsing page the page so why prevent partitioned indexes specifically? Andreas
On Wed, 14 Jan 2026 at 03:38, Andreas Karlsson <andreas@proxel.se> wrote:
> On 1/14/26 2:56 AM, Japin Li wrote:
>> Hi, hackers,
>> While working on pageinspect [0], I noticed that brin_page_items()
>> and
>> gist_page_items() only checked the access method (IS_BRIN/IS_GIST) but did
>> not verify that the passed relation is actually an index relation.
>> To make the check more robust and consistent with other pageinspect
>> index
>> functions (like btreefuncs.c, hashfuncs.c, etc.), the attached patch:
>> 1. Defines a local helper macro IS_INDEX(r) in both brinfuncs.c and
>> gistfuncs.c.
>> 2. Updates the error check to require both: the relation must be an
>> index and
>> use the expected access method.
>> The change is very small, low-risk, and only affects two functions
>> in
>> contrib/pageinspect.
>
> Since the two functions you touch are gist_page_items() and
> brin_page_items() is there actually any harm from being able to use
> the index definition from a partitioned when parsing the page? Seems
> unnecessary to prevent people from doing so unless I am missing
> something. It is not like we have any way to prevent the wrong index
> from being used when parsing page the page so why prevent partitioned
> indexes specifically?
>
Thanks for the thoughtful question!
The reason I added the IS_INDEX check (and reject partitioned indexes) is that
a partitioned index in PostgreSQL doesn't actually store any data pages itself
it only exists as a logical parent for the partition-level indexes. So passing
a partitioned index OID to brin_page_items() or gist_page_items() would
fundamentally not make sense: there are no real pages to inspect, and trying
to read a page from it would fail.
postgres=# SELECT * FROM brin_page_items(get_raw_page('brin_test_create_at_idx', 0), 'brin_test_create_at_idx');
ERROR: cannot get raw page from relation "brin_test_create_at_idx"
DETAIL: This operation is not supported for partitioned indexes.
OTOH, this is also fully consistent with how other pageinspect functions (like
btree_page_items, hash_page_items, etc.) behave — they only accept concrete,
non-partitioned index relations that actually have physical pages.
--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
Hi On Wed, 14 Jan 2026 at 17:40, Japin Li <japinli@hotmail.com> wrote: > > On Wed, 14 Jan 2026 at 03:38, Andreas Karlsson <andreas@proxel.se> wrote: > > On 1/14/26 2:56 AM, Japin Li wrote: > >> Hi, hackers, > >> While working on pageinspect [0], I noticed that brin_page_items() > >> and > >> gist_page_items() only checked the access method (IS_BRIN/IS_GIST) but did > >> not verify that the passed relation is actually an index relation. > >> To make the check more robust and consistent with other pageinspect > >> index > >> functions (like btreefuncs.c, hashfuncs.c, etc.), the attached patch: > >> 1. Defines a local helper macro IS_INDEX(r) in both brinfuncs.c and > >> gistfuncs.c. > >> 2. Updates the error check to require both: the relation must be an > >> index and > >> use the expected access method. > >> The change is very small, low-risk, and only affects two functions > >> in > >> contrib/pageinspect. > > > > Since the two functions you touch are gist_page_items() and > > brin_page_items() is there actually any harm from being able to use > > the index definition from a partitioned when parsing the page? Seems > > unnecessary to prevent people from doing so unless I am missing > > something. It is not like we have any way to prevent the wrong index > > from being used when parsing page the page so why prevent partitioned > > indexes specifically? > > > > Thanks for the thoughtful question! > > The reason I added the IS_INDEX check (and reject partitioned indexes) is that > a partitioned index in PostgreSQL doesn't actually store any data pages itself > it only exists as a logical parent for the partition-level indexes. So passing > a partitioned index OID to brin_page_items() or gist_page_items() would > fundamentally not make sense: there are no real pages to inspect, and trying > to read a page from it would fail. Correct, we do not need to run page inspect functions against partitioned indexes. Also, maybe we define this as pageinspect.h, like in [0] ? [0] https://www.postgresql.org/message-id/CALdSSPj-ADRgBk1_gspb2Q0eY2wxQHLfiWfFOmAwSxMF_AboRQ%40mail.gmail.com -- Best regards, Kirill Reshke
On Wed, 14 Jan 2026 at 17:53, Kirill Reshke <reshkekirill@gmail.com> wrote: > Hi > > On Wed, 14 Jan 2026 at 17:40, Japin Li <japinli@hotmail.com> wrote: >> >> On Wed, 14 Jan 2026 at 03:38, Andreas Karlsson <andreas@proxel.se> wrote: >> > On 1/14/26 2:56 AM, Japin Li wrote: >> >> Hi, hackers, >> >> While working on pageinspect [0], I noticed that brin_page_items() >> >> and >> >> gist_page_items() only checked the access method (IS_BRIN/IS_GIST) but did >> >> not verify that the passed relation is actually an index relation. >> >> To make the check more robust and consistent with other pageinspect >> >> index >> >> functions (like btreefuncs.c, hashfuncs.c, etc.), the attached patch: >> >> 1. Defines a local helper macro IS_INDEX(r) in both brinfuncs.c and >> >> gistfuncs.c. >> >> 2. Updates the error check to require both: the relation must be an >> >> index and >> >> use the expected access method. >> >> The change is very small, low-risk, and only affects two functions >> >> in >> >> contrib/pageinspect. >> > >> > Since the two functions you touch are gist_page_items() and >> > brin_page_items() is there actually any harm from being able to use >> > the index definition from a partitioned when parsing the page? Seems >> > unnecessary to prevent people from doing so unless I am missing >> > something. It is not like we have any way to prevent the wrong index >> > from being used when parsing page the page so why prevent partitioned >> > indexes specifically? >> > >> >> Thanks for the thoughtful question! >> >> The reason I added the IS_INDEX check (and reject partitioned indexes) is that >> a partitioned index in PostgreSQL doesn't actually store any data pages itself >> it only exists as a logical parent for the partition-level indexes. So passing >> a partitioned index OID to brin_page_items() or gist_page_items() would >> fundamentally not make sense: there are no real pages to inspect, and trying >> to read a page from it would fail. > > Correct, we do not need to run page inspect functions against > partitioned indexes. > > Also, maybe we define this as pageinspect.h, like in [0] ? > > [0] https://www.postgresql.org/message-id/CALdSSPj-ADRgBk1_gspb2Q0eY2wxQHLfiWfFOmAwSxMF_AboRQ%40mail.gmail.com > Yeah, I actually prepared a patch for this in [0]. Or we could address it in this thread if you prefer. What do you think? -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
On Wed, 14 Jan 2026 at 19:55, Japin Li <japinli@hotmail.com> wrote: > > > Or we could address it in this thread if you prefer. What do you think? > As you wish - we can continue here, because this is a separate change which has its independent value. -- Best regards, Kirill Reshke
On Wed, 14 Jan 2026 at 20:07, Kirill Reshke <reshkekirill@gmail.com> wrote: > On Wed, 14 Jan 2026 at 19:55, Japin Li <japinli@hotmail.com> wrote: >> >> >> Or we could address it in this thread if you prefer. What do you think? >> > > As you wish - we can continue here, because this is a separate change > which has its independent value. > Attach the v2 patch. 1. Move the IS_INDEX macro to pageinspect.h 2. Check relation kind for both brin and gist index -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Вложения
On Wed, 14 Jan 2026 at 23:31, Japin Li <japinli@hotmail.com> wrote: > On Wed, 14 Jan 2026 at 20:07, Kirill Reshke <reshkekirill@gmail.com> wrote: >> On Wed, 14 Jan 2026 at 19:55, Japin Li <japinli@hotmail.com> wrote: >>> >>> >>> Or we could address it in this thread if you prefer. What do you think? >>> >> >> As you wish - we can continue here, because this is a separate change >> which has its independent value. >> > > Attach the v2 patch. > > 1. Move the IS_INDEX macro to pageinspect.h > 2. Check relation kind for both brin and gist index > Sorry about that — I uploaded the wrong patch by mistake. Here's v3 attached. Thanks for your patience! -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Вложения
On 2026-Jan-14, Japin Li wrote: > -#define IS_INDEX(r) ((r)->rd_rel->relkind == RELKIND_INDEX) > -#define IS_BTREE(r) ((r)->rd_rel->relam == BTREE_AM_OID) > +#define IS_BTREE(r) (IS_INDEX(r) && (r)->rd_rel->relam == BTREE_AM_OID) I find this coding rather pointless. You can more easily do something like #define IS_BTREE(r) ((r)->rd_rel->relkind == RELKIND_INDEX && (r)->rd_rel->relam == BTREE_AM_OID) and get rid of the IS_INDEX macro completely, if it's not used anywhere else. Same for all the other index AMs. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On Wed, 14 Jan 2026 at 16:40, Álvaro Herrera <alvherre@kurilemu.de> wrote: > On 2026-Jan-14, Japin Li wrote: > >> -#define IS_INDEX(r) ((r)->rd_rel->relkind == RELKIND_INDEX) >> -#define IS_BTREE(r) ((r)->rd_rel->relam == BTREE_AM_OID) >> +#define IS_BTREE(r) (IS_INDEX(r) && (r)->rd_rel->relam == BTREE_AM_OID) > > I find this coding rather pointless. You can more easily do something > like > > #define IS_BTREE(r) ((r)->rd_rel->relkind == RELKIND_INDEX && (r)->rd_rel->relam == BTREE_AM_OID) > > and get rid of the IS_INDEX macro completely, if it's not used anywhere > else. Same for all the other index AMs. > Thanks for the review. I've fixed it according to your suggestion. > -- > Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Вложения
On 2026-Jan-15, Japin Li wrote: > Thanks for the review. > > I've fixed it according to your suggestion. Thanks! Unless somebody hates this, I'll get it pushed quickly. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
On Wed, Jan 14, 2026 at 06:54:17PM +0100, Alvaro Herrera wrote: > Thanks! Unless somebody hates this, I'll get it pushed quickly. That looks fine by me, at quick glance, removing some code churn. -- Michael
Вложения
After looking at this more closely, I wonder if this is really doing
what we want. For BRIN and GiST, you can only pass an index to the
brin_page_items and gist_page_items functions respectively, and that
only as to let the function know what relation the page comes from. The
actual read from the index comes from get_raw_page().
So in the regression database, I created
create index on regress_constr_partitioned using brin (a);
and then tried this
select * from brin_page_items(get_raw_page('regress_constr_partitioned_a_idx1', 1),
'regress_constr_partition1_a_idx'::regclass);
this gives me the following existing error:
ERROR: cannot get raw page from relation "regress_constr_partitioned_a_idx1"
DETALLE: This operation is not supported for partitioned indexes.
but if I instead do it the other way around,
select * from brin_page_items(get_raw_page('regress_constr_partition1_a_idx', 1),
'regress_constr_partitioned_a_idx1'::regclass);
the error is now
ERROR: "regress_constr_partitioned_a_idx1" is not a BRIN index
I wonder ... shouldn't these reports be more similar? Or, there's also
the alternative view that we don't _need_ to throw an error here. If I
remove the new check, I get this
select * from brin_page_items(get_raw_page('regress_constr_partition1_a_idx', 2),
'regress_constr_partitioned_a_idx1'::regclass);
itemoffset │ blknum │ attnum │ allnulls │ hasnulls │ placeholder │ empty │ value
────────────┼────────┼────────┼──────────┼──────────┼─────────────┼───────┼───────
1 │ 0 │ 1 │ t │ f │ f │ t │
(1 fila)
which seems ... perfectly okay? I mean, why are you worried about this?
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Computing is too important to be left to men." (Karen Spärck Jones)
On 1/15/26 8:57 PM, Álvaro Herrera wrote: > which seems ... perfectly okay? I mean, why are you worried about this? +1 I don't see the issue we are trying to solve either. Why not just let people run something totally harmless and fine? Andreas
On Thu, 15 Jan 2026 at 20:57, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> After looking at this more closely, I wonder if this is really doing
> what we want. For BRIN and GiST, you can only pass an index to the
> brin_page_items and gist_page_items functions respectively, and that
> only as to let the function know what relation the page comes from. The
> actual read from the index comes from get_raw_page().
>
> So in the regression database, I created
> create index on regress_constr_partitioned using brin (a);
>
> and then tried this
>
> select * from brin_page_items(get_raw_page('regress_constr_partitioned_a_idx1', 1),
'regress_constr_partition1_a_idx'::regclass);
>
> this gives me the following existing error:
> ERROR: cannot get raw page from relation "regress_constr_partitioned_a_idx1"
> DETALLE: This operation is not supported for partitioned indexes.
>
> but if I instead do it the other way around,
>
> select * from brin_page_items(get_raw_page('regress_constr_partition1_a_idx', 1),
'regress_constr_partitioned_a_idx1'::regclass);
>
> the error is now
> ERROR: "regress_constr_partitioned_a_idx1" is not a BRIN index
>
> I wonder ... shouldn't these reports be more similar? Or, there's also
> the alternative view that we don't _need_ to throw an error here. If I
> remove the new check, I get this
>
> select * from brin_page_items(get_raw_page('regress_constr_partition1_a_idx', 2),
'regress_constr_partitioned_a_idx1'::regclass);
> itemoffset │ blknum │ attnum │ allnulls │ hasnulls │ placeholder │ empty │ value
> ────────────┼────────┼────────┼──────────┼──────────┼─────────────┼───────┼───────
> 1 │ 0 │ 1 │ t │ f │ f │ t │
> (1 fila)
>
> which seems ... perfectly okay? I mean, why are you worried about this?
>
Thanks for the test. After rethinking, I agree — that BRIN/GIST index-type
check is useless.
OTOH, why don't we just do it the same way as the btree index functions?
brin_page_items(relname text, blkno bigint);
> --
> Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
> "Computing is too important to be left to men." (Karen Spärck Jones)
--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
On 2026-Jan-16, Japin Li wrote: > Thanks for the test. After rethinking, I agree — that BRIN/GIST index-type > check is useless. > > OTOH, why don't we just do it the same way as the btree index functions? > > brin_page_items(relname text, blkno bigint); Operational usefulness. If you have a corrupt page in a production, you can extract it from there with get_raw_page() and move it to another system to do low-level investigation. If you only have the interface you suggest, you force the researcher to access the production system (and run potentially dangerous tools), which may be best avoided. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On Fri, 16 Jan 2026 at 09:55, Álvaro Herrera <alvherre@kurilemu.de> wrote: > On 2026-Jan-16, Japin Li wrote: > >> Thanks for the test. After rethinking, I agree — that BRIN/GIST index-type >> check is useless. >> >> OTOH, why don't we just do it the same way as the btree index functions? >> >> brin_page_items(relname text, blkno bigint); > > Operational usefulness. If you have a corrupt page in a production, you > can extract it from there with get_raw_page() and move it to another > system to do low-level investigation. If you only have the interface > you suggest, you force the researcher to access the production system > (and run potentially dangerous tools), which may be best avoided. > Sorry, I didn't get your mind. We can still use get_raw_page(). > -- > Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
On 2026-Jan-20, Japin Li wrote: > Sorry, I didn't get your mind. We can still use get_raw_page(). My mind ATM is that there's nothing to be done here, but feel free to propose specific changes you're thinking about. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ Are you not unsure you want to delete Firefox? [Not unsure] [Not not unsure] [Cancel] http://smylers.hates-software.com/2008/01/03/566e45b2.html