Re: Microvacuum support for Hash Index

On Wed, Mar 15, 2017 at 2:10 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> Okay, I have done the changes as suggested by you. Please refer to the
> attached v8 patch. Thanks.

Cool, but this doesn't look right:

+    action = XLogReadBufferForRedo(record, 0, &buffer);
+
+    if (!IsBufferCleanupOK(buffer))
+        elog(PANIC, "hash_xlog_vacuum_one_page: failed to acquire
cleanup lock");

That could fail, I think, because of a pin from a Hot Standby backend.
You want to call XLogReadBufferForRedoExtended() with a third argument
of true. Come to think of it, shouldn't hash_xlog_split_allocate_page
be changed the same way?

+            /*
+             * Let us mark the page as clean if vacuum removes the DEAD tuples
+             * from an index page. We do this by clearing
LH_PAGE_HAS_DEAD_TUPLES
+             * flag.
+             */

Maybe add: Clearing this flag is just a hint; replay won't redo this.

+     * Hash Index records that are marked as LP_DEAD and being removed during
+     * hash index tuple insertion can conflict with standby queries.You might

The word Index shouldn't be capitalized here.  There should be a space
before "You".

The formatting of this comment is oddly narrow:

+ * _hash_vacuum_one_page - vacuum just one index page.
+ * Try to remove LP_DEAD items from the given page.  We
+ * must acquire cleanup lock on the page being modified
+ * before calling this function.

I'd add a blank line after the first line and reflow the rest to be
something more like 75 characters.  pgindent evidently doesn't think
this needs reformatting, but it's oddly narrow.

I suggest changing the header comment of
hash_xlog_vacuum_get_latestRemovedXid like this:

+ * Get the latestRemovedXid from the heap pages pointed at by the index
+ * tuples being deleted.  See also btree_xlog_delete_get_latestRemovedXid,
+ * on which this function is based.

This is looking good.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company