On Mon, Jun 20, 2016 at 7:28 AM, Asif Naeem <anaeem.it@gmail.com> wrote:
> Thank you for useful suggestions. PFA patch, I have tried to cover all the
> points mentioned.
Thanks for the new patch. I think that you have failed to address
this point from my previous review:
# I see why you changed the calling convention for visibilitymap_pin()
# and RecordPageWithFreeSpace(), but that's awfully invasive. I wonder
# if there's a better way to do that, like maybe having vacuumlazy.c ask
# the VM and FSM for their length in pages and then not trying to use
# those functions for block numbers that are too large.
The patch has gotten a lot smaller, and that's clearly good, but
introducing extended versions of those functions still seems like a
thing we should try to avoid. In particular, there's no way this hunk
is going to be acceptable:
@@ -286,6 +299,10 @@ visibilitymap_set(Relation rel, BlockNumber
heapBlk, Buffer heapBuf, if (BufferIsValid(heapBuf) && BufferGetBlockNumber(heapBuf) != heapBlk) elog(ERROR,
"wrongheap buffer passed to visibilitymap_set");
+ /* In case of invalid buffer just return */
+ if(vmBuf == InvalidBuffer)
+ return;
+ /* Check that we have the right VM page pinned */ if (!BufferIsValid(vmBuf) || BufferGetBlockNumber(vmBuf) !=
mapBlock) elog(ERROR, "wrong VM buffer passed to visibilitymap_set");
You're going to have to find a different approach there.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company