Visibility map page pinned for too long ?

I was looking at the code when the following tiny bit caught my attention.
In vacuumlazy.c, we release the pin on the final VM page at line number 972.

954 if (vacrelstats->num_dead_tuples > 0)
955 {
956 /* Log cleanup info before we touch indexes */
957 vacuum_log_cleanup_info(onerel, vacrelstats);
958
959 /* Remove index entries */
960 for (i = 0; i < nindexes; i++)
961 lazy_vacuum_index(Irel[i],
962 &indstats[i],
963 vacrelstats);
964 /* Remove tuples from heap */
965 lazy_vacuum_heap(onerel, vacrelstats);
966 vacrelstats->num_index_scans++;
967 }
968
969 /* Release the pin on the visibility map page */
970 if (BufferIsValid(vmbuffer))
971 {
972 ReleaseBuffer(vmbuffer);
973 vmbuffer = InvalidBuffer;
974 }

So we are holding the pin right through the index vacuuming and the second
pass over the heap; both can take a very long time. We can and should
really be releasing the pin *before* those steps. In fact, it would be
appropriate to do it right after the preceding big for-loop.

While it may or may not matter from the performance or correctness
perspective, I think we should fix that.

Thanks,
Pavan

--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee

On 3 December 2012 17:37, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com> wrote:
> I was looking at the code when the following tiny bit caught my attention.
> In vacuumlazy.c, we release the pin on the final VM page at line number 972.
>
> 954 if (vacrelstats->num_dead_tuples > 0)
> 955 {
> 956 /* Log cleanup info before we touch indexes */
> 957 vacuum_log_cleanup_info(onerel, vacrelstats);
> 958
> 959 /* Remove index entries */
> 960 for (i = 0; i < nindexes; i++)
> 961 lazy_vacuum_index(Irel[i],
> 962 &indstats[i],
> 963 vacrelstats);
> 964 /* Remove tuples from heap */
> 965 lazy_vacuum_heap(onerel, vacrelstats);
> 966 vacrelstats->num_index_scans++;
> 967 }
> 968
> 969 /* Release the pin on the visibility map page */
> 970 if (BufferIsValid(vmbuffer))
> 971 {
> 972 ReleaseBuffer(vmbuffer);
> 973 vmbuffer = InvalidBuffer;
> 974 }
>
> So we are holding the pin right through the index vacuuming and the second
> pass over the heap; both can take a very long time. We can and should really
> be releasing the pin *before* those steps. In fact, it would be appropriate
> to do it right after the preceding big for-loop.
>
> While it may or may not matter from the performance or correctness
> perspective, I think we should fix that.

Yes, its a clear bug. Patched.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services