Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Avoid unnecessary GinFormTuple() calls for incompressible posting lists.

  1. GIN tries to form a tuple with a partial compressedList during insertion

    Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> — 2025-04-30T08:40:26Z

    Hi,

In the functions addItemPointersToLeafTuple and buildFreshLeafTuple
(in gininsert.c), the result of ginCompressPostingList()
is passed to GinFormTuple without checking whether
ginCompressPostingList() successfully packed all items.
These GinFormTuple calls consistently fail because the resulting
tuples always exceed the maximum size.
While this doesn’t result in data corruption, it might still be worth
addressing.
Additionally, the condition if (compressedList) is unnecessary, since
ginCompressPostingList() never returns NULL.

Please find the attached patch fixing it.

Best regards,
Arseniy Mukhin

  2. Re: GIN tries to form a tuple with a partial compressedList during insertion

    Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> — 2025-07-02T19:40:32Z

    Hi!

Here is a new version. I added a commit message. I will add it to PG19-2.


Best regards,
Arseniy Mukhin

  3. Re: GIN tries to form a tuple with a partial compressedList during insertion

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-09-25T22:20:59Z

    On Wed, Jul 2, 2025 at 12:41 PM Arseniy Mukhin
<arseniy.mukhin.dev@gmail.com> wrote:
>
> Hi!
>
> Here is a new version. I added a commit message. I will add it to PG19-2.

Thank you for the patch.

I think the proposed change is reasonable; if we fail to compress all
ItemPointers, it doesn't make sense to try to form a tuple from it.

Here are some review comments:

---
-       compressedList = ginCompressPostingList(newItems, newNPosting,
GinMaxItemSize,
-
                 NULL);
+       compressedList = ginCompressPostingList(newItems, newNPosting,
GinMaxItemSize - GinGetPostingOffset(old),
+
                 &nwritten);

Why does it need to subtract GinGetPostingOffset(old) from the maxsize?

---
        pfree(newItems);
-       if (compressedList)
+       if (nwritten == newNPosting)
        {
                res = GinFormTuple(ginstate, attnum, key, category,
                                                   (char *) compressedList,

SizeOfGinPostingList(compressedList),
                                                   newNPosting,
                                                   false);
-               pfree(compressedList);
        }
+       pfree(compressedList);

I think it would be cleaner if we move 'pfree(newItems)' to before
'pfree(compressedList)'.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

  4. Re: GIN tries to form a tuple with a partial compressedList during insertion

    Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> — 2025-09-26T08:58:34Z

    Hi,

On Fri, Sep 26, 2025 at 1:21 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Jul 2, 2025 at 12:41 PM Arseniy Mukhin
> <arseniy.mukhin.dev@gmail.com> wrote:
> >
> > Hi!
> >
> > Here is a new version. I added a commit message. I will add it to PG19-2.
>
> Thank you for the patch.
>

Thank you for looking into this!

> I think the proposed change is reasonable; if we fail to compress all
> ItemPointers, it doesn't make sense to try to form a tuple from it.
>
> Here are some review comments:
>
> ---
> -       compressedList = ginCompressPostingList(newItems, newNPosting,
> GinMaxItemSize,
> -
>                  NULL);
> +       compressedList = ginCompressPostingList(newItems, newNPosting,
> GinMaxItemSize - GinGetPostingOffset(old),
> +
>                  &nwritten);
>
> Why does it need to subtract GinGetPostingOffset(old) from the maxsize?
>

While writing the patch I realized that there is a room for small
optimization. The idea here is that as we know the index tuple size
limit and we know how much space we need for everything except the
posting list (we can get it with GinGetPostingOffset(old)), then we
can calculate the size limit for the posting list more precisely. But
now I think it was a bad idea to add this optimization to the fix.
Even if it relates to the same line of the code, it's a different
thing and it's confusing. Sorry about that. I removed it from the
patch.

> ---
>         pfree(newItems);
> -       if (compressedList)
> +       if (nwritten == newNPosting)
>         {
>                 res = GinFormTuple(ginstate, attnum, key, category,
>                                                    (char *) compressedList,
>
> SizeOfGinPostingList(compressedList),
>                                                    newNPosting,
>                                                    false);
> -               pfree(compressedList);
>         }
> +       pfree(compressedList);
>
> I think it would be cleaner if we move 'pfree(newItems)' to before
> 'pfree(compressedList)'.
>

Done.

Please find the attached new version.

Thank you!


Best regards,
Arseniy Mukhin

  5. Re: GIN tries to form a tuple with a partial compressedList during insertion

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-10-06T21:03:44Z

    On Fri, Sep 26, 2025 at 1:58 AM Arseniy Mukhin
<arseniy.mukhin.dev@gmail.com> wrote:
>
> Hi,
>
> On Fri, Sep 26, 2025 at 1:21 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Jul 2, 2025 at 12:41 PM Arseniy Mukhin
> > <arseniy.mukhin.dev@gmail.com> wrote:
> > >
> > > Hi!
> > >
> > > Here is a new version. I added a commit message. I will add it to PG19-2.
> >
> > Thank you for the patch.
> >
>
> Thank you for looking into this!
>
> > I think the proposed change is reasonable; if we fail to compress all
> > ItemPointers, it doesn't make sense to try to form a tuple from it.
> >
> > Here are some review comments:
> >
> > ---
> > -       compressedList = ginCompressPostingList(newItems, newNPosting,
> > GinMaxItemSize,
> > -
> >                  NULL);
> > +       compressedList = ginCompressPostingList(newItems, newNPosting,
> > GinMaxItemSize - GinGetPostingOffset(old),
> > +
> >                  &nwritten);
> >
> > Why does it need to subtract GinGetPostingOffset(old) from the maxsize?
> >
>
> While writing the patch I realized that there is a room for small
> optimization. The idea here is that as we know the index tuple size
> limit and we know how much space we need for everything except the
> posting list (we can get it with GinGetPostingOffset(old)), then we
> can calculate the size limit for the posting list more precisely. But
> now I think it was a bad idea to add this optimization to the fix.
> Even if it relates to the same line of the code, it's a different
> thing and it's confusing. Sorry about that. I removed it from the
> patch.
>
> > ---
> >         pfree(newItems);
> > -       if (compressedList)
> > +       if (nwritten == newNPosting)
> >         {
> >                 res = GinFormTuple(ginstate, attnum, key, category,
> >                                                    (char *) compressedList,
> >
> > SizeOfGinPostingList(compressedList),
> >                                                    newNPosting,
> >                                                    false);
> > -               pfree(compressedList);
> >         }
> > +       pfree(compressedList);
> >
> > I think it would be cleaner if we move 'pfree(newItems)' to before
> > 'pfree(compressedList)'.
> >
>
> Done.
>
> Please find the attached new version.
>

Thank you for updating the patch! LGTM, so I've pushed.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

  6. Re: GIN tries to form a tuple with a partial compressedList during insertion

    Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> — 2025-10-07T07:06:41Z

    On Tue, Oct 7, 2025 at 12:04 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> ...
> Thank you for updating the patch! LGTM, so I've pushed.
>

Thank you!


Best regards,
Arseniy Mukhin