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.
