Re: [HACKERS] Write Ahead Logging for Hash Indexes

On Mon, Feb 27, 2017 at 11:07 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Feb 16, 2017 at 8:16 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> Attached are refactoring patches.  WAL patch needs some changes based
>> on above comments, so will post it later.
>
> After some study, I have committed 0001, and also committed 0002 and
> 0003 as a single commit, with only minor tweaks.
>
> I haven't made a full study of 0004 yet, but I'm a bit concerned about
> a couple of the hunks, specifically this one:
>
> @@ -985,9 +962,9 @@ _hash_splitbucket_guts(Relation rel,
>
>                  if (PageGetFreeSpace(npage) < itemsz)
>                  {
> -                    /* write out nbuf and drop lock, but keep pin */
> -                    MarkBufferDirty(nbuf);
> +                    /* drop lock, but keep pin */
>                      LockBuffer(nbuf, BUFFER_LOCK_UNLOCK);
> +
>                      /* chain to a new overflow page */
>                      nbuf = _hash_addovflpage(rel, metabuf, nbuf,
> (nbuf == bucket_nbuf) ? true : false);
>                      npage = BufferGetPage(nbuf);
>
> And also this one:
>
> @@ -1041,17 +1024,6 @@ _hash_splitbucket_guts(Relation rel,
>       * To avoid deadlocks due to locking order of buckets, first lock the old
>       * bucket and then the new bucket.
>       */
> -    if (nbuf == bucket_nbuf)
> -    {
> -        MarkBufferDirty(bucket_nbuf);
> -        LockBuffer(bucket_nbuf, BUFFER_LOCK_UNLOCK);
> -    }
> -    else
> -    {
> -        MarkBufferDirty(nbuf);
> -        _hash_relbuf(rel, nbuf);
> -    }
> -
>      LockBuffer(bucket_obuf, BUFFER_LOCK_EXCLUSIVE);
>      opage = BufferGetPage(bucket_obuf);
>      oopaque = (HashPageOpaque) PageGetSpecialPointer(opage);
>
> I haven't quite grasped what's going on here, but it looks like those
> MarkBufferDirty() calls didn't move someplace else, but rather just
> vanished.  That would seem to be not good.
>

Yeah, actually those were added later in Enable-WAL-for-Hash* patch,
but I think as this patch is standalone, so we should not remove it
from their existing usage, I have added those back and rebased the
remaining patches.


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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers