Re: [HACKERS] pgsql 10: hash indexes testing

On Fri, Aug 4, 2017 at 11:15 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Aug 4, 2017 at 6:22 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> I have implemented the patch with this approach as other approach
>> require quite extensive changes which I am not sure is the right thing
>> to do at this stage.
>
> I think this approach is actually better anyway.  There's no guarantee
> that VACUUM can be responsive enough to get the job done in time, work
> items or no work items, and the split-cleanup is cheap enough that I
> don't think we ought to worry about negative effects on foreground
> workloads.  Sure, it could have some impact, but *less bloat* could
> also have some impact in the other direction.
>
> +        hashbucketcleanup(rel, obucket, bucket_obuf,
> +                          BufferGetBlockNumber(bucket_obuf), NULL,
> +                          maxbucket, highmask, lowmask, NULL, NULL, true,
> +                          NULL, NULL);
> +        LockBuffer(bucket_nbuf, BUFFER_LOCK_UNLOCK);
>
> Don't we want to do those things in the other order?
>

Earlier one of the callers was releasing the old bucket lock first, so
I just maintained that order, but I think it is better if we release
new bucket lock first. I think that will maintain the ordering and it
is better to do it before actual cleanup of the old bucket as cleanup
can take some time and there is no use of retaining a lock on the new
bucket for that time.  I will do some testing after making the change
and will post a patch in some time.

> - * which is passed in buffer nbuf, pinned and write-locked.  That lock and
> - * pin are released here.  (The API is set up this way because we must do
> - * _hash_getnewbuf() before releasing the metapage write lock.  So instead of
> - * passing the new bucket's start block number, we pass an actual buffer.)
> + * which is passed in buffer nbuf, pinned and write-locked.  The lock will be
> + * released here and pin must be released by the caller.  (The API is set up
> + * this way because we must do _hash_getnewbuf() before releasing the metapage
> + * write lock.  So instead of passing the new bucket's start block number, we
> + * pass an actual buffer.)
>
> Isn't the parenthesized part at the end of the comment wrong?  I
> realize this patch isn't editing that part anyway except for
> reflowing, but further up in that same block comment it says this:
>
>  * The caller must hold a pin, but no lock, on the metapage buffer.
>  * The buffer is returned in the same state.  (The metapage is only
>  * touched if it becomes necessary to add or remove overflow pages.)
>

The comment doesn't seem to be wrong but is not making much sense
here.  Both the actions _hash_getnewbuf and release of metapage lock
is done in the caller.  In 9.6, we were passing old bucket's block
number and new bucket's buffer, so the comment tries to explain why it
is passing buffer for the new bucket.  It makes less sense now because
we are passing buffer for both old and new buckets, so we can remove
it.  Do you want me to remove it as part of this patch or as a
separate patch?

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