Re: mdnblocks() sabotages error checking in _mdfd_getseg()

On 2015-12-09 16:50:06 -0500, Robert Haas wrote:
> Now, if subsequent to this an index scan happens to sweep through and
> try to fetch a block in 123456.2, it will work!  This happens because
> _mdfd_getseg() doesn't care about the length of the segments; it only
> cares whether or not they exist.

Unless in recovery in the startup process, or when EXTENSION_CREATE is
passed to it. Depending on whether it or mdnblocks were called first,
and depending on which segment is missing.  In that case it'd *possibly*
pad the last block in a sgment with zeroes, Yes, only the last block:         *         * We have to maintain the
invariantthat segments before the last         * active segment are of size RELSEG_SIZE; therefore, pad them out
* with zeroes if needed.  (This only matters if caller is         * extending the relation discontiguously, but that
canhappen in         * hash indexes.)         */        if (behavior == EXTENSION_CREATE || InRecovery)        {
   if (_mdnblocks(reln, forknum, v) < RELSEG_SIZE)            {                char       *zerobuf = palloc0(BLCKSZ);
 
                mdextend(reln, forknum,                         nextsegno * ((BlockNumber) RELSEG_SIZE) - 1,
            zerobuf, skipFsync);                pfree(zerobuf);            }            v->mdfd_chain =
_mdfd_openseg(reln,forknum, +nextsegno, O_CREAT);        }
 


> If 123456.1 were actually missing,
> then we'd never test whether 123456.2 exists and we'd get an error.
> But because mdnblocks() created 123456.1, _mdfd_getseg() is now quite
> happy to fetch blocks in 123456.2; it considers the empty 123456.1
> file to be a sufficient reason to look for 123456.2, and seeing that
> file, and finding the block it wants therein, it happily returns that
> block, blithely ignoring the fact that it passed over a completely .1
> segment before returning a block from .2.  This is maybe not the
> smartest thing ever.

I think it might be worthwhile to add a check against this. It'd not be
very hard to ensure the segment is indeed large enough iff further
segments exist. Possibly not an ERROR, but a WARNING might be useful. To
avoid overhead and absurd log spamming we should make that check only if
the the segment isn't already open.


> The comment in mdnblocks.c says this:
> 
>                          * Because we pass O_CREAT, we will create the
> next segment (with
>                          * zero length) immediately, if the last
> segment is of length
>                          * RELSEG_SIZE.  While perhaps not strictly
> necessary, this keeps
>                          * the logic simple.
> 
> I don't really see how this "keeps the logic simple".  What it does is
> allow sequential scans and index scans to have two completely
> different notions of how many accessible blocks there are in the
> relation.  Granted, this kind of thing really shouldn't happen, but
> sometimes bad things do happen.  Therefore, I propose the attached
> patch.

I'm in favor of changing this behaviour - it's indeed weird and somewhat
error prone.

Do you want to change this in master, or backpatch a change? To me the
code in md.c is subtle and complicated enough that I'm rather inclined
to only change this in master.


This code is really hard to test effectively :(.


Greetings,

Andres Freund