Hi,
On Sat, Apr 1, 2017 at 7:06 AM, Ashutosh Sharma <ashu.coe...@gmail.com> wrote:
> On Sat, Apr 1, 2017 at 6:00 AM, Robert Haas <robertmh...@gmail.com> wrote:
>> On Fri, Mar 31, 2017 at 6:09 PM, Ashutosh Sharma <ashu.coe...@gmail.com>
>> wrote:
>>> Well, That is another option but the main idea was to be inline with
>>> the btree code.
>>
>> That's not a bad goal in principal, but _bt_killitems() doesn't have
>> any similar argument.
>
> It was there but later got removed with some patch (may be the patch
> for reducing pinning and buffer content lock for btree scans). The
> following commit has this changes.
>
> commit 09cb5c0e7d6fbc9dee26dc429e4fc0f2a88e5272
> Author: Tom Lane <t...@sss.pgh.pa.us>
> Date: Sun May 7 01:21:30 2006 +0000
>
>>
>> (Also, speaking of consistency, why did we end up with
>> _hash_kill_items, with an underscore between kill and items, and
>> _bt_killitems, without one?)
>
> That is just to follow the naming convention in hash.h Please check
> the review comments for this at - [1].
>
>>
>>> Moreover, I am not sure if acquiring lwlock inside
>>> hashendscan (basically the place where we are trying to close down the
>>> things) would look good.
>>
>> Well, if that's not a good thing to do, hiding it inside some other
>> function doesn't make it better.
>
> okay, agreed. I will submit the patch very shortly.
As suggested, I am now acquiring lock inside the caller function.
Attached is the patch having this changes. Thanks.
>
>
> [1] -
> https://www.postgresql.org/message-id/cf6abd8a-77b5-f1c7-8e50-5bef461e0522%40redhat.com
--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 34cc08f..b835f77 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -476,9 +476,17 @@ hashrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
HashScanOpaque so = (HashScanOpaque) scan->opaque;
Relation rel = scan->indexRelation;
- /* Before leaving current page, deal with any killed items */
+ /*
+ * Before leaving current page, deal with any killed items.
+ * Also, ensure that we acquire lock on current page before
+ * calling _hash_kill_items.
+ */
if (so->numKilled > 0)
+ {
+ LockBuffer(so->hashso_curbuf, BUFFER_LOCK_SHARE);
_hash_kill_items(scan);
+ LockBuffer(so->hashso_curbuf, BUFFER_LOCK_UNLOCK);
+ }
_hash_dropscanbuf(rel, so);
@@ -507,9 +515,17 @@ hashendscan(IndexScanDesc scan)
HashScanOpaque so = (HashScanOpaque) scan->opaque;
Relation rel = scan->indexRelation;
- /* Before leaving current page, deal with any killed items */
+ /*
+ * Before leaving current page, deal with any killed items.
+ * Also, ensure that we acquire lock on current page before
+ * calling _hash_kill_items.
+ */
if (so->numKilled > 0)
+ {
+ LockBuffer(so->hashso_curbuf, BUFFER_LOCK_SHARE);
_hash_kill_items(scan);
+ LockBuffer(so->hashso_curbuf, BUFFER_LOCK_UNLOCK);
+ }
_hash_dropscanbuf(rel, so);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
