Обсуждение: What is HeapScanDescData.rs_initblock good for?

The BRIN patch added a HeapScanDescData field rs_initblock, but so far as
I can see it's utterly without use, and it's quite confusing (people might
mistake it for rs_startblock, for example).  Any objection to taking it
out again?
        regards, tom lane

Tom Lane wrote:
> The BRIN patch added a HeapScanDescData field rs_initblock, but so far as
> I can see it's utterly without use, and it's quite confusing (people might
> mistake it for rs_startblock, for example).  Any objection to taking it
> out again?

Ouch, you're right, my mistake.  Feel free to remove it, yeah.

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> The BRIN patch added a HeapScanDescData field rs_initblock, but so far as
>> I can see it's utterly without use, and it's quite confusing (people might
>> mistake it for rs_startblock, for example).  Any objection to taking it
>> out again?

> Ouch, you're right, my mistake.  Feel free to remove it, yeah.

... While I'm looking at it, it sure looks like the BRIN patch broke
syncscan for those index build methods that were using it, which was
most.  You've got IndexBuildHeapRangeScan unconditionally calling
heap_setscanlimits and thereby trashing the result of ss_get_location().

I'm inclined to let it call heap_setscanlimits only if not allow_sync.
        regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Tom Lane wrote:
> >> The BRIN patch added a HeapScanDescData field rs_initblock, but so far as
> >> I can see it's utterly without use, and it's quite confusing (people might
> >> mistake it for rs_startblock, for example).  Any objection to taking it
> >> out again?
> 
> > Ouch, you're right, my mistake.  Feel free to remove it, yeah.
> 
> ... While I'm looking at it, it sure looks like the BRIN patch broke
> syncscan for those index build methods that were using it, which was
> most.  You've got IndexBuildHeapRangeScan unconditionally calling
> heap_setscanlimits and thereby trashing the result of ss_get_location().

Hmm, right, I failed to notice that.

> I'm inclined to let it call heap_setscanlimits only if not allow_sync.

It is possible for a partial range scan to join an existing herd of
scans that happens to be processing that part of the table, in which
case this wouldn't be sufficient.  However, two considerations: 1) range
scans, at least for BRIN, aren't normally large enough for synscans to
matter all that much; and 2) it would require additional code.  So I'm
inclined to do it as you suggest, which is simplest.

(I think this is what the rs_initblock thing was for BTW: set up an
initial block number other than 0 for the range scan, so that when it
reached the end in a syncscan that started further ahead, it knew what
block to "overflow" back to.)

One scenario in which the sync scan could matter is initial index
creation.

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> I'm inclined to let it call heap_setscanlimits only if not allow_sync.

> It is possible for a partial range scan to join an existing herd of
> scans that happens to be processing that part of the table, in which
> case this wouldn't be sufficient.  However, two considerations: 1) range
> scans, at least for BRIN, aren't normally large enough for synscans to
> matter all that much; and 2) it would require additional code.  So I'm
> inclined to do it as you suggest, which is simplest.

I already did that, but feel free to whack it further if you're so
inclined.
        regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Tom Lane wrote:
> >> I'm inclined to let it call heap_setscanlimits only if not allow_sync.
> 
> > It is possible for a partial range scan to join an existing herd of
> > scans that happens to be processing that part of the table, in which
> > case this wouldn't be sufficient.  However, two considerations: 1) range
> > scans, at least for BRIN, aren't normally large enough for synscans to
> > matter all that much; and 2) it would require additional code.  So I'm
> > inclined to do it as you suggest, which is simplest.
> 
> I already did that, but feel free to whack it further if you're so
> inclined.

Nah, I just failed to see your commit.

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