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.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
