Re: Reviewing freeze map code

On Wed, Jun 29, 2016 at 11:14 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Fri, Jun 24, 2016 at 11:04 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Fri, Jun 24, 2016 at 4:33 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>> On 2016-06-23 18:59:57 -0400, Alvaro Herrera wrote:
>>>> Andres Freund wrote:
>>>>
>>>> > I'm looking into three approaches right now:
>>>> >
>>>> > 3) Use WAL logging for the already_marked = true case.
>>>>
>>>>
>>>> > 3) This approach so far seems the best. It's possible to reuse the
>>>> > xl_heap_lock record (in an afaics backwards compatible manner), and in
>>>> > most cases the overhead isn't that large. It's of course annoying to
>>>> > emit more WAL, but it's not that big an overhead compared to extending a
>>>> > file, or to toasting. It's also by far the simplest fix.
>>>>
>>
>>>
>>
>> You are right, I think we can try such an optimization in Head and
>> that too if we see a performance hit with adding this new WAL in
>> heap_update.
>>
>>
>
> +1 for #3 approach, and attached draft patch for that.
> I think attached patch would fix this problem but please let me know
> if this patch is not what you're thinking.
>

Review comments:

+ if (RelationNeedsWAL(relation))
+ {
+ xl_heap_lock xlrec;
+ XLogRecPtr recptr;
+
..
+ xlrec.offnum = ItemPointerGetOffsetNumber(&oldtup.t_self);
+ xlrec.locking_xid = xid;
+ xlrec.infobits_set = compute_infobits(oldtup.t_data->t_infomask,
+ oldtup.t_data->t_infomask2);
+ XLogRegisterData((char *) &xlrec, SizeOfHeapLock);
+ recptr = XLogInsert(RM_HEAP_ID, XLOG_HEAP_LOCK);
+ PageSetLSN(page, recptr);
+ }

There is nothing in this record which recorded the information about
visibility clear flag. How will you ensure to clear the flag after
crash? Have you considered to log cid using log_heap_new_cid() for
logical decoding?

It seems to me that the value of locking_xid should be xmax_old_tuple,
why you have chosen xid?

+ /* Celar PD_ALL_VISIBLE flags */
+ if (PageIsAllVisible(BufferGetPage(buffer)))
+ {
+ all_visible_cleared = true;
+ PageClearAllVisible(BufferGetPage(buffer));
+ visibilitymap_clear(relation, BufferGetBlockNumber(buffer),
+ vmbuffer);
+ }
+
+ MarkBufferDirty(buffer);
+

/* Clear obsolete visibility flags ... */
oldtup.t_data->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);

I think it is better to first update tuple related info and then clear
PD_ALL_VISIBLE flags (for order, refer how we have done in heap_update
in the code below where you are trying to add new code).

Couple of typo's -
/relasing/releasing
/Celar/Clear

I think in this approach, it is important to measure the performance
of update, may be you can use simple-update option of pgbench for
various workloads. Try it with different fill factors (-F fillfactor
option in pgbench).

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