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

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