On Mon, Apr 19, 2021 at 1:57 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Mon, Apr 19, 2021 at 5:04 PM Kyotaro Horiguchi > <horikyota....@gmail.com> wrote: > > > > At Mon, 19 Apr 2021 13:32:31 +0900, Masahiko Sawada <sawada.m...@gmail.com> > > wrote in > > > On Fri, Apr 16, 2021 at 12:16 PM Kyotaro Horiguchi > > > <horikyota....@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 With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
