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. > 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. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
copy_freeze_v4.patch
Description: Binary data
