Re: Re: PANIC: invalid index offnum: 186 when processing BRIN indexes in VACUUM

Tom Lane wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> 
> > Yeah, I think this approach results in better code.  The attached patch
> > implements that, and it passes the test for me (incl. calling
> > brin_summarize_new_values concurrently with vacuum, when running the
> > insert; the former does include the final page range whereas the latter
> > does not.)
> 
> Hm, so IIUC the point is that once the placeholder tuple is in, we can
> rely on concurrent inserters to update it for insertions into pages that
> are added after we determine our scan stop point.  But if the scan stop
> point is chosen before inserting the placeholder, then we have a race
> condition.

Exactly.  We don't need to scan those pages once the placeholder tuple
is in.

> The given code seems a brick or so short of a load, though: if the table
> has been extended sufficiently, it could compute scanNumBlks as larger
> than bs_pagesPerRange, no?  You need to clamp the computation result.

Oops, right.

> Also, shouldn't the passed-in heapBlk always be a multiple of
> pagesPerRange already?

Yeah, I guess I can turn that into an assert.

> Do we still need the complication in brinsummarize to discriminate
> against the last partial range?  Now that the lock consideration
> is gone, I think that might be a wart.

You mean this code?

		/*
		 * Unless requested to summarize even a partial range, go away now if
		 * we think the next range is partial.
		 *
		 * Maybe the table already grew to cover this range completely, but we
		 * don't want to spend a whole RelationGetNumberOfBlocks to find out,
		 * so just leave it for next time.
		 */
		if (!include_partial &&
			(startBlk + pagesPerRange > heapNumBlocks))
			break;

In the case of VACUUM, it's not desirable to create a summarization for
the last partial range, because if the table is still being filled, that
would slow down the insertion process.  So we pass include_partial=false
there.  In brin_summarize_new_values, the theory is that the user called
that function because they're done loading (at least temporarily) so
it's better to process the partial range.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services