Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits

Thank you for sharing the updated patch!

On Tue, Mar 26, 2019 at 6:26 PM Pavan Deolasee <pavan.deolasee@gmail.com> wrote:
>
>
> On Fri, Mar 22, 2019 at 12:19 PM Masahiko Sawada <sawada.mshk@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