Re: [BUGS] Breakage with VACUUM ANALYSE + partitions - Mailing list pgsql-hackers

At 2016-04-12 09:00:57 -0400, robertmhaas@gmail.com wrote:
>
> On Mon, Apr 11, 2016 at 1:17 PM, Andres Freund <andres@anarazel.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'tcreate 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?

-- Abhijit

diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 578276d..58ea5a0 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -1783,6 +1783,8 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,               if
(v->mdfd_chain== NULL)               {
 
+                       bool thisSegNeedsExtension = _mdnblocks(reln, forknum, v) < RELSEG_SIZE;
+                       /*                        * Normally we will create new segments only if authorized by the
                 * caller (i.e., we are doing mdextend()).  But when doing WAL
 
@@ -1799,7 +1801,7 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,                        */
                    if (behavior == EXTENSION_CREATE || InRecovery)                       {
 
-                               if (_mdnblocks(reln, forknum, v) < RELSEG_SIZE)
+                               if (thisSegNeedsExtension)                               {
        char       *zerobuf = palloc0(BLCKSZ);
 
@@ -1810,7 +1812,7 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
  }                               v->mdfd_chain = _mdfd_openseg(reln, forknum, +nextsegno, O_CREAT);
  }
 
-                       else
+                       else if (!thisSegNeedsExtension)                       {                               /* We
won'tcreate segment if not existent */                               v->mdfd_chain = _mdfd_openseg(reln, forknum,
nextsegno,0);