Re: [BUGS] Breakage with VACUUM ANALYSE + partitions

On 22 April 2016 at 18:07, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-04-14 11:09:29 +0530, Abhijit Menon-Sen wrote:
>> At 2016-04-12 09:00:57 -0400, robertmhaas(at)gmail(dot)com wrote:
>> >
>> > On Mon, Apr 11, 2016 at 1:17 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> > >
>> > > 3) Actually handle the case of the last open segment not being
>> > > RELSEG_SIZE properly in _mdfd_getseg() - mdnblocks() does so.
>> >
>> > #3 seems like it's probably about 15 years overdue, so let's do that
>> > anyway.
>>
>> I don't have anything useful to contribute, I'm just trying to
>> understand this fix.
>>
>> _mdfd_getseg() looks like this:
>>
>> targetseg = blkno / ((BlockNumber) RELSEG_SIZE);
>> for (nextsegno = 1; nextsegno <= targetseg; nextsegno++)
>> {
>> Assert(nextsegno == v->mdfd_segno + 1);
>>
>> if (v->mdfd_chain == NULL)
>> {
>> /*
>> * Normally we will create new segments only if authorized by the
>> * caller (i.e., we are doing mdextend()). […]
>> */
>> if (behavior == EXTENSION_CREATE || InRecovery)
>> {
>> if (_mdnblocks(reln, forknum, v) < RELSEG_SIZE)
>> /* mdextend */
>> v->mdfd_chain = _mdfd_openseg(reln, forknum, +nextsegno, O_CREAT);
>> }
>> else
>> {
>> /* We won't create segment if not existent */
>> v->mdfd_chain = _mdfd_openseg(reln, forknum, nextsegno, 0);
>> }
>> if (v->mdfd_chain == NULL)
>> /* return NULL or ERROR */
>> }
>> v = v->mdfd_chain;
>> }
>>
>> Do I understand correctly that the case of the "last open segment"
>> (i.e., the one for which mdfd_chain == NULL) not being RELSEG_SIZE
>> (i.e., _mdnblocks(reln, forknum, v) < RELSEG_SIZE) should not call
>> _mfdf_openseg on nextsegno if behaviour is not EXTENSION_CREATE or
>> InRecovery?
>>
>> And that "We won't create segment if not existent" should happen, but
>> doesn't only because the next segment file wasn't removed earlier, so
>> we have to add an extra check for that case?
>>
>> In other words, is something like the following what's needed here, or
>> is there more to it?
>
> It's a bit more complicated than that, but that's the principle. Thanks
> for the patch, and sorry for my late response. See my attached version
> for a more fleshed out version of this.
>
> Looking at the code around this I found:
> * _mdfd_getseg(), in contrast to mdnblocks() doesn't check segment size,
> neither whether to continue with a too small, nor to error out with a
> too big segment
> * For, imo pretty bad, reasons in mdsync() we currently rely on
> EXTENSION_RETURN_NULL to extend the file in recovery, and to set errno
> to ENOENT. Brrr.
> * we currently FATAL if a segment is too big - I've copied that
> behaviour, but why not just ERROR out?
>
> The attached patch basically adds the segment size checks to
> _mdfd_getseg(), and doesn't perform extension, even in recovery, if
> EXTENSION_REALLY_RETURN_NULL is passed.
>
> This fixes the issue for me, both in the originally reported variant,
> and in some more rudely setup version (i.e. rm'ing files).

Fixes it for me too.

Thom