On Wed, Jan 18, 2017 at 11:51 AM, Mithun Cy <mithun...@enterprisedb.com> wrote:
> On Tue, Jan 17, 2017 at 10:07 AM, Amit Kapila <amit.kapil...@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.

@@ -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.

+_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

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

Reply via email to