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?
- * 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
metapageis only* touched if it becomes necessary to add or remove overflow pages.)
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company