Re: pgsql 10: hash indexes testing

On Fri, Aug 4, 2017 at 11:45 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Aug 4, 2017 at 11:15 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Aug 4, 2017 at 6:22 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>

Changed as per suggestion.

>> - * 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?
>

I have not done anything for this comment as it doesn't sound wrong to
me. I think it is not making much sense in the current code and we
can remove it or change it as part of the separate patch if you or
others think so.

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