Re: [HACKERS] Microvacuum support for Hash Index - Mailing list pgsql-hackers

On Tue, Mar 14, 2017 at 8:02 AM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> Attached is the v6 patch for microvacuum in hash index rebased on top
> of 'v10 patch for WAL in hash index - [1]' and 'v1 patch for WAL
> consistency check for hash index - [2]'.
>
> [1] - https://www.postgresql.org/message-id/CAA4eK1%2Bk5wR4-kAjPqLoKemuHayQd6RkQQT9gheTfpn%2B72o1UA%40mail.gmail.com
> [2] -
https://www.postgresql.org/message-id/flat/CAGz5QCJLERUn_zoO0eDv6_Y_d0o4tNTMPeR7ivTLBg4rUrJdwg@mail.gmail.com#CAGz5QCJLERUn_zoO0eDv6_Y_d0o4tNTMPeR7ivTLBg4rUrJdwg@mail.gmail.com
>
> Also, the patch (mask_hint_bit_LH_PAGE_HAS_DEAD_TUPLES.patch) to mask
> 'LH_PAGE_HAS_DEAD_TUPLES' flag which got added as a part of
> Microvacuum patch is attached with this mail.

Generally, this patch looks like a pretty straightforward adaptation
of the similar btree mechanism to hash indexes, so if it works for
btree it ought to work here, too.  But I noticed a few things while
reading through it.

+        /* Get RelfileNode from relation OID */
+        rel = relation_open(htup->t_tableOid, NoLock);
+        rnode = rel->rd_node;
+        relation_close(rel, NoLock);        itup->t_tid = htup->t_self;
-        _hash_doinsert(index, itup);
+        _hash_doinsert(index, itup, rnode);

This is an awfully low-level place to be doing something like this.
I'm not sure exactly where this should be happening, but not in the
per-tuple callback.

+    /*
+     * If there's nothing running on the standby we don't need to derive a
+     * full latestRemovedXid value, so use a fast path out of here.  This
+     * returns InvalidTransactionId, and so will conflict with all HS
+     * transactions; but since we just worked out that that's zero people,
+     * it's OK.
+     */
+    if (CountDBBackends(InvalidOid) == 0)
+        return latestRemovedXid;

I see that this comment (and most of what surrounds it) was just
copied from the existing btree example, but isn't there a discrepancy
between the comment and the code?  It says it returns
InvalidTransactionId, but it doesn't.  Also, you dropped the XXX from
the btree original, and the following reachedConsistency check.

+     * Hash Index delete records can conflict with standby queries.You might
+     * think that vacuum records would conflict as well, but we've handled

But they're not called delete records in a hash index.  The function
is called hash_xlog_vacuum_one_page.  The corresponding btree function
is btree_xlog_delete.  So this comment needs a little more updating.

+            if (IsBufferCleanupOK(bucket_buf))
+            {
+                _hash_vacuum_one_page(rel, metabuf, buf, bucket_buf,
+                                      (buf == bucket_buf) ? true : false,
+                                      hnode);
+                if (bucket_buf != buf)
+                    LockBuffer(bucket_buf, BUFFER_LOCK_UNLOCK);
+
+                if (PageGetFreeSpace(page) >= itemsz)
+                    break;              /* OK, now we have enough space */
+            }

I might be missing something, but I don't quite see why this needs a
cleanup lock on the primary bucket page.  I would think a cleanup lock
on the page we're actually modifying would suffice, and I think if
that's correct it would be a better way to go.  If that's not correct,
then I think the comments needs some work.

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