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.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/