On Wed, Jan 18, 2017 at 11:51 AM, Mithun Cy <mithun.cy@enterprisedb.com> wrote:
> On Tue, Jan 17, 2017 at 10:07 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
>> (b) Another somewhat bigger problem is that with this new change it
>> won't retain the pin on meta page till the end which means we might
>> need to perform an I/O again during operation to fetch the meta page.
>> AFAICS, you have just changed it so that you can call new API
>> _hash_getcachedmetap, if that's true, then I think you have to find
>> some other way of doing it. BTW, why can't you design your new API
>> such that it always take pinned metapage? You can always release the
>> pin in the caller if required. I understand that you don't always
>> need a metapage in that API, but I think the current usage of that API
>> is also not that good.
>
>
> -- Yes what you say is right. I wanted to make _hash_getcachedmetap
> API self-sufficient. But always 2 possible consecutive reads for
> metapage are connected as we pin the page to buffer to avoid I/O. Now
> redesigned the API such a way that caller pass pinned meta page if we
> want to set the metapage cache; So this gives us the flexibility to
> use the cached meta page data in different places.
>
1.
@@ -505,26 +505,22 @@ hashbulkdelete(IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
..
..
+ metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_NOLOCK, LH_META_PAGE);
+ cachedmetap = _hash_getcachedmetap(rel, metabuf);
In the above flow, do we really need an updated metapage, can't we use
the cached one? We are already taking care of bucket split down in
that function.
2.
+HashMetaPage
+_hash_getcachedmetap(Relation rel, Buffer metabuf)
+{
..
..
+ if (BufferIsInvalid(metabuf))
+ return (HashMetaPage) rel->rd_amcache;
..
+_hash_getbucketbuf_from_hashkey(Relation rel, uint32 hashkey, int access,
+ HashMetaPage *cachedmetap)
{
..
+ if (!(metap = _hash_getcachedmetap(rel, InvalidBuffer)))
+ {
+ metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_NOLOCK, LH_META_PAGE);
+ metap = _hash_getcachedmetap(rel, metabuf);
+ Assert(metap != NULL);
+ }
..
}
The above two chunks of code look worse as compare to your previous
patch. I think what we can do is keep the patch ready with both the
versions of _hash_getcachedmetap API (as you have in _v11 and _v12)
and let committer take the final call.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com