Thank you for sharing the updated patch! On Tue, Mar 26, 2019 at 6:26 PM Pavan Deolasee <pavan.deola...@gmail.com> wrote: > > > On Fri, Mar 22, 2019 at 12:19 PM Masahiko Sawada <sawada.m...@gmail.com> > wrote: >> >> I've looked at the patch and have comments and questions. >> >> + /* >> + * While we are holding the lock on the page, check if all tuples >> + * in the page are marked frozen at insertion. We can safely mark >> + * such page all-visible and set visibility map bits too. >> + */ >> + if (CheckPageIsAllFrozen(relation, buffer)) >> + PageSetAllVisible(page); >> + >> + MarkBufferDirty(buffer); >> >> Maybe we don't need to mark the buffer dirty if the page is not set >> all-visible. >> > > Yeah, makes sense. Fixed. > >> >> If we have CheckAndSetPageAllVisible() for only this >> situation we would rather need to check that the VM status of the page >> should be 0 and then set two flags to the page? The 'flags' will >> always be (VISIBILITYMAP_ALL_FROZEN | VISIBILITYMAP_ALL_VISIBLE) in >> copy freeze case. I'm confused that this function has both code that >> assumes some special situations and code that can be used in generic >> situations. > > > If a second COPY FREEZE is run within the same transaction and if starts > inserting into the page used by the previous COPY FREEZE, then the page will > already be marked all-visible/all-frozen. So we can skip repeating the > operation again. While it's quite unlikely that someone will do that and I > can't think of a situation where only one of those flags will be set, I don't > see a harm in keeping the code as is. This code is borrowed from vacuumlazy.c > and at some point we can even move it to some common location.
Thank you for explanation, agreed. > >> >> Perhaps we can add some tests for this feature to pg_visibility module. >> > > That's a good idea. Please see if the tests included in the attached patch > are enough. > The patch looks good to me. There is no comment from me. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
