Re: visibility maps and heap_prune - Mailing list pgsql-hackers

On Mon, Dec 8, 2008 at 06:56, Pavan Deolasee<pavan.deolasee@gmail.com> wrote:
> Here is a patch which implements this. The PD_ALL_VISIBLE flag is set if all
> tuples in the page are visible to all transactions and there are no DEAD
> line pointers in the page. The second check is required so that VACUUM takes
> up the page. We could slightly distinguish the two cases (one where the page
> requires vacuuming only because of DEAD line pointers and the other where
> the page-tuples do not require any visibility checks), but I thought its not
> worth the complexity.

Hi!

I was round robin assigned to review this.  So take my comments with
the grain of salt (or novice HOT salt) they deserve.

I did some quick performance testing that basically boiled down to:
insert
(hot) update
select

to see if I could detect any noticeable performance difference (see
attachments for more detail for exact queries ran, all run with
autovac off).

The only major difference was with this patch vacuum time (after the
first select after some hot updates) was significantly reduced for my
test case (366ms vs 16494ms).

There was no noticeable (within noise) select or update slow down.

I was able to trigger WARNING:  PD_ALL_VISIBLE flag once while running
pgbench but have not be able to re-create it... (should I keep
trying?)

See comments on patch below...

>Index: src/backend/access/heap/pruneheap.c

<snip>

>*************** heap_page_prune_opt(Relation relation, B
>*** 118,125 ****
>           (void) heap_page_prune(relation, buffer, OldestXmin, false, true);
>       }
>
>!      /* And release buffer lock */
>!      LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
>   }
>  }
>
>--- 124,150 ----
>           (void) heap_page_prune(relation, buffer, OldestXmin, false, true);
>       }
>
>!      /*
>!       * Since the visibility map page may require an I/O,release the buffer
>!       * lock before updating the visibility map.
>!       */

Would it be worth having heap_page_prune() return or pass in a ptr so
we can saw we need to update the visibility map because we set/changed
PageIsAllVisible?

>!      if (PageIsAllVisible(page))
>!      {
>!          Buffer vmbuffer = InvalidBuffer;
>!          /* Release buffer lock */
>!          LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
>!
>!          visibilitymap_pin(relation, BufferGetBlockNumber(buffer), &vmbuffer);
>!          LockBuffer(buffer, BUFFER_LOCK_SHARE);
>!          if (PageIsAllVisible(page))
>!              visibilitymap_set(relation, BufferGetBlockNumber(buffer),
>!                                PageGetLSN(page), &vmbuffer);
>!          LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
>!          if (BufferIsValid(vmbuffer))
>!              ReleaseBuffer(vmbuffer);
>!      }
>!      else
>!          LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
>   }
>  }
>

<snip>

>*************** heap_page_prune(Relation relation, Buffe
>*** 245,250 ****
>--- 277,291 ----
>        */
>       PageClearFull(page);
>
>+      /* Update the all-visible flag on the page */
>+      if (!PageIsAllVisible(page) && prstate.all_visible)
>+          PageSetAllVisible(page);
>+      else if (PageIsAllVisible(page) && !prstate.all_visible)
>+      {
>+          elog(WARNING, "PD_ALL_VISIBLE flag was incorrectly set");
>+          PageClearAllVisible(page);

Hrm do we need to update the visibility map ? AFAICT this wont update
it it because we only check for PageIsAllVisible() in
heap_page_prune_opt().

>+      }
>+
>       MarkBufferDirty(buffer);
>
>       /*
>*************** heap_page_prune(Relation relation, Buffe
>*** 282,287 ****
>--- 323,341 ----
>            PageClearFull(page);
>            SetBufferCommitInfoNeedsSave(buffer);
>        }
>+
>+       /* Update the all-visible flag on the page */
>+       if (!PageIsAllVisible(page) && prstate.all_visible)
>+       {
>+           PageSetAllVisible(page);
>+           SetBufferCommitInfoNeedsSave(buffer);
>+       }
>+       else if (PageIsAllVisible(page) && !prstate.all_visible)
>+       {
>+           elog(WARNING, "PD_ALL_VISIBLE flag was incorrectly set");
>+           PageClearAllVisible(page);
>+           SetBufferCommitInfoNeedsSave(buffer);

Same question as above.

>+      }
>   }
>
>   END_CRIT_SECTION();

<snip>

>*************** heap_prune_chain(Relation relation, Buff
>*** 495,503 ****
>--- 557,596 ----
>                   */
>                  heap_prune_record_prunable(prstate,
>                                             HeapTupleHeaderGetXmax(htup));
>+                 prstate->all_visible = false;
>                  break;
>
>              case HEAPTUPLE_LIVE:
>+                 /*
>+                  * Is the tuple definitely visible to all transactions?
>+                  *
>+                  * NB: Like with per-tuple hint bits, we can't set the
>+                  * PD_ALL_VISIBLE flag if the inserter committed
>+                  * asynchronously. See SetHintBits for more info. Check
>+                  * that the HEAP_XMIN_COMMITTED hint bit is set because of
>+                  * that.
>+                  */
>+                 if (prstate->all_visible)
>+                 {
>+                     TransactionId xmin;
>+
>+                     if (!(htup->t_infomask & HEAP_XMIN_COMMITTED))
>+                     {
>+                         prstate->all_visible = false;
>+                         break;
>+                     }
>+                     /*
>+                      * The inserter definitely committed. But is it
>+                      * old enough that everyone sees it as committed?
>+                      */
>+                     xmin = HeapTupleHeaderGetXmin(htup);
>+                     if (!TransactionIdPrecedes(xmin, OldestXmin))
>+                     {
>+                         prstate->all_visible = false;
>+                         break;
>+                     }
>+                 }
>+                 break;

(nitpick) missing newline

>              case HEAPTUPLE_INSERT_IN_PROGRESS:
>
>                  /*>