Обсуждение: GiST nitpicks I want to discuss (and maybe eventually fix)

Hi hackers!

There are several things in GiST that trigger me when I think about
them. They are minor by themselves and maybe are not worth the trouble
fixing alone... But I decided to create a thread to discuss if we
should do anything about them.


0001:
Remove GistTuplesDeleted support from GiST

This bit flag value is unused since
68446b2c87a2aee5d8c2eb2aade7bb6d4195b7e1.
F_TUPLES_DELETED flag is not removed, since it is still used for
pageinspect. This is because we will still have this flag in mask
after a major upgrade, so we should keep page inspect code that
displays it. Patch merely suggests not to set it anymore.

(there was 0002, but I self-rejected it, because it is too picky)

0003:

Remove block field from gistxlogPageReuse walrecord.
GIST_REUSE_PAGE is spectail WAL record. It does not reference any
page, does not change any page in redo, its sole purpose is to kill
standby-queries which might still observe GiST index page we want to
reuse. It has `block` field which is simply not used for any purpose.
I propose to remove this field, reducing WAL footprint by an enormous
4 byte-per-record.

0004:
Reduce GIST_MAX_SPLIT_PAGES to some sane limit.

Value of GIST_MAX_SPLIT_PAGES is too big. There is no chance that GiST
page split will successfully produce 75 new pages, because there is an
upper limit of how many backup blocks a single WAL record can
reference (XLR_MAX_BLOCK_ID = 32). Patch reduces to
GIST_MAX_SPLIT_PAGES and adds static assertion for value.

Some history:
commit introducing GIST_MAX_SPLIT_PAGES [0]
commit introducing XLR_MAX_BLOCK_ID [1]

[0] was committed before [1]



I was able to create a case where split produces 6 pages. I was not
able to do more. So, I dont know if there is any real-life case where
this problem reproduces.


[0] https://github.com/postgres/postgres/commit/04e298b826d4
[0] https://github.com/postgres/postgres/commit/2c03216d8311


-- 
Best regards,
Kirill Reshke

Hi Kirill!

> On 6 Oct 2025, at 22:57, Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> 0001:
> Remove GistTuplesDeleted support from GiST
>
> This bit flag value is unused since
> 68446b2c87a2aee5d8c2eb2aade7bb6d4195b7e1.
> F_TUPLES_DELETED flag is not removed, since it is still used for
> pageinspect. This is because we will still have this flag in mask
> after a major upgrade, so we should keep page inspect code that
> displays it. Patch merely suggests not to set it anymore.

Makes sense. I hope one day we will add a catalog field to track index creation version. This would pave the way to get
ridof invalid GiST tuples and return this flag too. We can use it for something better. 
FTR, in commit message you could mention shorter version of hash 68446b2.

>
> (there was 0002, but I self-rejected it, because it is too picky)

Out of curiosity, what was that?

>
> 0003:
>
> Remove block field from gistxlogPageReuse walrecord.
> GIST_REUSE_PAGE is spectail WAL record. It does not reference any
> page, does not change any page in redo, its sole purpose is to kill
> standby-queries which might still observe GiST index page we want to
> reuse. It has `block` field which is simply not used for any purpose.
> I propose to remove this field, reducing WAL footprint by an enormous
> 4 byte-per-record.

Yes, this is Hot-standby-only record. In offline conversation you mentioned that "RelFileLocator locator;" is not
needed,only "Oid dbOid;". Maybe let's save a little more bytes? 

>
> 0004:
> Reduce GIST_MAX_SPLIT_PAGES to some sane limit.
>
> Value of GIST_MAX_SPLIT_PAGES is too big. There is no chance that GiST
> page split will successfully produce 75 new pages, because there is an
> upper limit of how many backup blocks a single WAL record can
> reference (XLR_MAX_BLOCK_ID = 32). Patch reduces to
> GIST_MAX_SPLIT_PAGES and adds static assertion for value.
>
> Some history:
> commit introducing GIST_MAX_SPLIT_PAGES [0]
> commit introducing XLR_MAX_BLOCK_ID [1]
>
> [0] was committed before [1]
>
>
>
> I was able to create a case where split produces 6 pages. I was not
> able to do more. So, I dont know if there is any real-life case where
> this problem reproduces.

IMHO, static assert is too fancy. Why not just #define GIST_MAX_SPLIT_PAGES (XLR_MAX_BLOCK_ID - 1) ?


Overall changes seem valuable, reducing GiST code complexity. Thank you!


Best regards, Andrey Borodin.

On Mon, 6 Oct 2025 at 23:53, Andrey Borodin <x4mmm@yandex-team.ru> wrote:

Thank you for review

> Makes sense. I hope one day we will add a catalog field to track index creation version. This would pave the way to
getrid of invalid GiST tuples and return this flag too. We can use it for something better.
 

In the GIN index we have an index creation version, and it is placed
on the metapage. So does btree, if i'm not mistaken . So, the index
version is stored in data, not in catalog. I doubt we will support two
technologies for one purpose.  Since GiST index has no metapage, I
doubt we will be successful here.

>
> Yes, this is Hot-standby-only record. In offline conversation you mentioned that "RelFileLocator locator;" is not
needed,only "Oid dbOid;". Maybe let's save a little more bytes?
 


It is about ResolveRecoveryConflictWithSnapshot. This function
RelFileLocator arg and only uses database oid. Changing this is too
out-of-scope.





-- 
Best regards,
Kirill Reshke

Hi,

On Tue, Oct 07, 2025 at 12:02:21AM +0500, Kirill Reshke wrote:
> On Mon, 6 Oct 2025 at 23:53, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > Makes sense. I hope one day we will add a catalog field to track
> > index creation version. This would pave the way to get rid of
> > invalid GiST tuples and return this flag too. We can use it for
> > something better.
> 
> In the GIN index we have an index creation version, and it is placed
> on the metapage. So does btree, if i'm not mistaken . So, the index
> version is stored in data, not in catalog. I doubt we will support two
> technologies for one purpose.  Since GiST index has no metapage, I
> doubt we will be successful here.

I always found it klunky that you need (AFAIK) an extension
(pageinspect) to figure out the btree/gin index version via its
meta-page and ISTM that this would be useful information to be stored in
the catalog.


Michael