On Mon, Apr 19, 2021 at 8:04 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Apr 19, 2021 at 1:57 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Apr 19, 2021 at 5:04 PM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote:
> > >
> > > At Mon, 19 Apr 2021 13:32:31 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
> > > > On Fri, Apr 16, 2021 at 12:16 PM Kyotaro Horiguchi
> > > > <horikyota.ntt@gmail.com> wrote:
> > > > > AFAICS the page is always empty when RelationGetBufferForTuple
> > > > > returned a valid vmbuffer. So the "if" should be an "assert" instead.
> > > >
> > > > There is a chance that RelationGetBufferForTuple() returns a valid
> > > > vmbuffer but the page is not empty, since RelationGetBufferForTuple()
> > > > checks without a lock if the page is empty. But when it comes to
> > > > HEAP_INSERT_FROZEN cases it actually doesn’t happen at least for now
> > > > since only one process inserts tuples into the relation. Will fix.
> > >
> > > Yes. It seems to me that it is cleaner that RelationGetBufferForTuple
> > > returns vmbuffer only when the caller needs to change vm state.
> > > Thanks.
> > >
> > > > > And, the patch changes the value of all_frozen_set to false when the
> > > > > page was already all-frozen (thus not empty). It would be fine since
> > > > > we don't need to change the visibility of the page in that case but
> > > > > the variable name is no longer correct. set_all_visible or such?
> > > >
> > > > It seems to me that the variable name all_frozen_set corresponds to
> > > > XLH_INSERT_ALL_FROZEN_SET but I see your point. How about
> > > > set_all_frozen instead since we set all-frozen bits (also implying
> > > > setting all-visible)?
> > >
> > > Right. However, "if (set_all_frozen) then "set all_visible" looks like
> > > a bug^^;. all_frozen_set looks better in that context than
> > > set_all_frozen. So I withdraw the comment.
> > >
> > > > BTW I found the following description of XLH_INSERT_ALL_FROZEN_SET but
> > > > there is no all_visible_set anywhere:
> > > >
> > > > /* all_frozen_set always implies all_visible_set */
> > > > #define XLH_INSERT_ALL_FROZEN_SET (1<<5)
> > > >
> > > > I'll update those comments as well.
> > >
> > > FWIW, it seems like a shorthand of "ALL_FROZEN_SET implies ALL_VISIBLE
> > > to be set together". The current comment is working to me.
> > >
> >
> > Okay, I've updated the patch accordingly. Please review it.
>
> I was reading the patch, just found some typos: it should be "a frozen
> tuple" not "an frozen tuple".
>
> + * Also pin visibility map page if we're inserting an frozen tuple into
> + * If we're inserting an frozen entry into empty page, pin the
Thank you for the comment.
I’ve updated the patch including the above comment.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
