Re: Reviewing freeze map code

On Thu, Jul 14, 2016 at 11:36 AM, Andres Freund <andres@anarazel.de> wrote:
> Hi,
>
> Master does
>                 /* temporarily make it look not-updated */
>                 oldtup.t_data->t_ctid = oldtup.t_self;
> here, and as is the wal record won't reflect that, because:
> static void
> heap_xlog_lock(XLogReaderState *record)
> {
> ...
>                 /*
>                  * Clear relevant update flags, but only if the modified infomask says
>                  * there's no update.
>                  */
>                 if (HEAP_XMAX_IS_LOCKED_ONLY(htup->t_infomask))
>                 {
>                         HeapTupleHeaderClearHotUpdated(htup);
>                         /* Make sure there is no forward chain link in t_ctid */
>                         ItemPointerSet(&htup->t_ctid,
>                                                    BufferGetBlockNumber(buffer),
>                                                    offnum);
>                 }
> won't enter the branch, because HEAP_XMAX_LOCK_ONLY won't be set.  Which
> will leave t_ctid and HEAP_HOT_UPDATED set differently on the master and
> standby / after crash recovery.   I'm failing to see any harmful
> consequences right now, but differences between master and standby are a bad
> thing. Pre 9.3 that's not a problem, we reset ctid and HOT_UPDATED
> unconditionally there.   I think I'm more comfortable with setting
> HEAP_XMAX_LOCK_ONLY until the tuple is finally updated - that also
> coincides more closely with the actual meaning.
>

Just thinking out loud.  If we set HEAP_XMAX_LOCK_ONLY during update,
then won't it impact the return value of
HeapTupleHeaderIsOnlyLocked().  It will start returning true whereas
otherwise I think it would have returned false due to in_progress
transaction.   As  HeapTupleHeaderIsOnlyLocked() is being used at many
places, it might impact those cases, I have not checked in deep
whether such an impact would cause any real issue, but it seems to me
that some analysis is needed there unless you think we are safe with
respect to that.

> Any arguments against?
>
>>
>> +             /* Clear only the all-frozen bit on visibility map if needed */
>> +             if (PageIsAllVisible(BufferGetPage(buffer)) &&
>> +                     VM_ALL_FROZEN(relation, block, &vmbuffer))
>> +             {
>> +                     visibilitymap_clear_extended(relation, block, vmbuffer,
>> +                                                                              VISIBILITYMAP_ALL_FROZEN);
>> +             }
>> +
>
> FWIW, I don't think it's worth introducing visibilitymap_clear_extended.
> As this is a 9.6 only patch, i think it's better to change
> visibilitymap_clear's API.
>
> Unless somebody protests I'm planning to commit with those adjustments
> tomorrow.
>

Do you think performance tests done by Sawada-san are sufficient to
proceed here?



-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com