From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Jesper Pedersen <jesper(dot)pedersen(at)redhat(dot)com>, Jeff Janes <jeff(dot)janes(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Write Ahead Logging for Hash Indexes |
Date: | 2017-02-28 14:01:39 |
Message-ID: | CAA4eK1+mvCucroWQwX3S7aBR=0yBJGF+jQz4x4Cx9QVsMFTZUw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Feb 27, 2017 at 11:07 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Feb 16, 2017 at 8:16 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
0001-Restructure-split-bucket-code.patch | application/octet-stream | 9.2 KB |
0002-Restructure-hash-index-creation.patch | application/octet-stream | 14.5 KB |
0003-Enable-WAL-for-Hash-Indexes.patch | application/octet-stream | 95.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2017-02-28 14:04:29 | Re: Disallowing multiple queries per PQexec() |
Previous Message | Magnus Hagander | 2017-02-28 13:57:56 | Re: port of INSTALL file generation to XSLT |