Re: Reviewing freeze map code - Mailing list pgsql-hackers
From | Amit Kapila |
---|---|
Subject | Re: Reviewing freeze map code |
Date | |
Msg-id | CAA4eK1JK3amfqZaoKJYBkHCzeAYj2hdY_yB0b9o2_uDtJbaV7w@mail.gmail.com Whole thread Raw |
In response to | Re: Reviewing freeze map code (Masahiko Sawada <sawada.mshk@gmail.com>) |
Responses |
Re: Reviewing freeze map code
|
List | pgsql-hackers |
On Wed, Jun 29, 2016 at 11:14 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > On Fri, Jun 24, 2016 at 11:04 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> On Fri, Jun 24, 2016 at 4:33 AM, Andres Freund <andres@anarazel.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
pgsql-hackers by date: