Thread: Print physical file path when checksum check fails

HHi,

On 2020-02-10 16:04:21 +0800, Hubert Zhang wrote:
> Currently we only print block number and relation path when checksum check
> fails. See example below:
> 
> ERROR: invalid page in block 333571 of relation base/65959/656195

> DBA complains that she needs additional work to calculate which physical
> file is broken, since one physical file can only contain `RELSEG_SIZE`
> number of blocks. For large tables, we need to use many physical files with
> additional suffix, e.g. 656195.1, 656195.2 ...
> 
> Is that a good idea to also print the physical file path in error message?
> Like below:
> 
> ERROR: invalid page in block 333571 of relation base/65959/656195, file
> path base/65959/656195.2

I think that'd be a nice improvement. But:

I don't think the way you did it is right architecturally. The
segmenting is really something that lives within md.c, and we shouldn't
further expose it outside of that. And e.g. the undo patchset uses files
with different segmentation - but still goes through bufmgr.c.

I wonder if this partially signals that the checksum verification piece
is architecturally done in the wrong place currently? It's imo not good
that every place doing an smgrread() needs to separately verify
checksums. OTOH, it doesn't really belong inside smgr.c.


This layering issue was also encountered in
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3eb77eba5a51780d5cf52cd66a9844cd4d26feb0
so perhaps we should work to reuse the FileTag it introduces to
represent segments, without hardcoding the specific segment size?

Regards,

Andres

> @@ -912,17 +912,20 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
>                  {
>                      ereport(WARNING,
>                              (errcode(ERRCODE_DATA_CORRUPTED),
> -                             errmsg("invalid page in block %u of relation %s; zeroing out page",
> +                             errmsg("invalid page in block %u of relation %s, "
> +                                    "file path %s; zeroing out page",
>                                      blockNum,
> -                                    relpath(smgr->smgr_rnode, forkNum))));
> +                                    relpath(smgr->smgr_rnode, forkNum),
> +                                    relfilepath(smgr->smgr_rnode, forkNum, blockNum))));
>                      MemSet((char *) bufBlock, 0, BLCKSZ);
>                  }
>                  else
>                      ereport(ERROR,
>                              (errcode(ERRCODE_DATA_CORRUPTED),
> -                             errmsg("invalid page in block %u of relation %s",
> +                             errmsg("invalid page in block %u of relation %s, file path %s",
>                                      blockNum,
> -                                    relpath(smgr->smgr_rnode, forkNum))));
> +                                    relpath(smgr->smgr_rnode, forkNum),
> +                                    relfilepath(smgr->smgr_rnode, forkNum, blockNum))));
>              }
>          }
>      }
> diff --git a/src/common/relpath.c b/src/common/relpath.c
> index ad733d1363..8b39c4ac4f 100644
> --- a/src/common/relpath.c
> +++ b/src/common/relpath.c
> @@ -208,3 +208,30 @@ GetRelationPath(Oid dbNode, Oid spcNode, Oid relNode,
>      }
>      return path;
>  }
> +
> +/*
> + * GetRelationFilePath - construct path to a relation's physical file
> + * given its block number.
> + */
> +    char *
> +GetRelationFilePath(Oid dbNode, Oid spcNode, Oid relNode,
> +                    int backendId, ForkNumber forkNumber, BlockNumber blkno)
> +{
> +    char       *path;
> +    char       *fullpath;
> +    BlockNumber    segno;
> +
> +    path = GetRelationPath(dbNode, spcNode, relNode, backendId, forkNumber);
> +
> +    segno = blkno / ((BlockNumber) RELSEG_SIZE);
> +
> +    if (segno > 0)
> +    {
> +        fullpath = psprintf("%s.%u", path, segno);
> +        pfree(path);
> +    }
> +    else
> +        fullpath = path;
> +
> +    return fullpath;
> +}

Greetings,

Andres Freund

Hello. Thank you for the new patch.

At Tue, 18 Feb 2020 09:27:39 +0800, Hubert Zhang <hzhang@pivotal.io> wrote in 
> On Wed, Feb 12, 2020 at 5:22 PM Hubert Zhang <hzhang@pivotal.io> wrote:
> 
> > Thanks Andres,
> >
> > On Tue, Feb 11, 2020 at 5:30 AM Andres Freund <andres@anarazel.de> wrote:
> >
> >> HHi,
> >>
> >> On 2020-02-10 16:04:21 +0800, Hubert Zhang wrote:
> >> > Currently we only print block number and relation path when checksum
> >> check
> >> > fails. See example below:
> >> >
> >> > ERROR: invalid page in block 333571 of relation base/65959/656195
> >>
> >> > DBA complains that she needs additional work to calculate which physical
> >> > file is broken, since one physical file can only contain `RELSEG_SIZE`
> >> > number of blocks. For large tables, we need to use many physical files
> >> with
> >> > additional suffix, e.g. 656195.1, 656195.2 ...
> >> >
> >> > Is that a good idea to also print the physical file path in error
> >> message?
> >> > Like below:
> >> >
> >> > ERROR: invalid page in block 333571 of relation base/65959/656195, file
> >> > path base/65959/656195.2
> >>
> >> I think that'd be a nice improvement. But:
> >>
> >> I don't think the way you did it is right architecturally. The
> >> segmenting is really something that lives within md.c, and we shouldn't
> >> further expose it outside of that. And e.g. the undo patchset uses files
> >> with different segmentation - but still goes through bufmgr.c.
> >>
> >> I wonder if this partially signals that the checksum verification piece
> >> is architecturally done in the wrong place currently? It's imo not good
> >> that every place doing an smgrread() needs to separately verify
> >> checksums. OTOH, it doesn't really belong inside smgr.c.
> >>
> >>
> >> This layering issue was also encountered in
> >>
> >> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3eb77eba5a51780d5cf52cd66a9844cd4d26feb0
> >> so perhaps we should work to reuse the FileTag it introduces to
> >> represent segments, without hardcoding the specific segment size?
> >>
> >>
> > I checked the FileTag commit. It calls `register_xxx_segment` inside md.c
> > to store the sync request into a hashtable and used by checkpointer later.
> >
> > Checksum verify is simpler. We could move the `PageIsVerified` into md.c
> > (mdread). But we can not elog error inside md.c because read buffer mode
> > RBM_ZERO_ON_ERROR is at bugmgr.c level.
> >
> > One idea is to change save the error message(or the FileTag) at (e.g. a
> > static string in bufmgr.c) by calling `register_checksum_failure` inside
> > mdread in md.c.
> >
> > As for your concern about the need to do checksum verify after every
> > smgrread, we now move the checksum verify logic into md.c, but we still
> > need to check the checksum verify result after smgrread and reset buffer to
> > zero if mode is RBM_ZERO_ON_ERROR.
> >
> > If this idea is OK, I will submit the new PR.
> >
> >
> Based on Andres's comments, here is the new patch for moving checksum
> verify logic into mdread() instead of call PageIsVerified in every
> smgrread(). Also using FileTag to print the physical file name when
> checksum verify failed, which handle segmenting inside md.c as well.

The patch doesn't address the first comment from Andres. It still
expose the notion of segment to the upper layer, but bufmgr (bufmgr.c
and bufpage.c) or Realation (relpath.c) layer shouldn't know of
segment.  So the two layers should ask smgr without using segment
number for the real file name for the block.

For example, I think the following structure works. (Without moving
checksum verification.)

======
md.c:
  char *mdfname(SmgrRelation reln, Forknumber forkNum, BlockNumber blocknum);
smgr.c:
  char *smgrfname(SMgrRelation reln, ForkNumber forkNum, BlockNumber Blocknum);

bufmgr.c:
  ReadBuffer_common()
  {
  ..
            /* check for garbage data */
            if (!PageIsVerified((Page) bufBlock, blockNum))
                if (mode == RBM_ZERO_ON_ERROR || zero_damaged_pages)
                    ereport(WARNING,
                             errmsg("invalid page in block %u in file %s; zeroing out page",
                                    blockNum,
                                    smgrfname(smgr, forkNum, blockNum))));
                    MemSet((char *) bufBlock, 0, BLCKSZ);
====

However, the block number in the error messages looks odd as it is
actually not the block number in the segment. We could convert
BlockNum into relative block number or offset in the file but it would
be overkill. So something like this works?

"invalid page in block %u in relation %u, file \"%s\"",
   blockNum, smgr->smgr_rnode.node.relNode, smgrfname()


If we also verify checksum in md layer, callback is overkill since the
immediate caller consumes the event immediately.  We can signal the
error by somehow returning a file tag.

======
md.c:
  FileTag *mdread(...)   /* or void mdread(..., FileTag*)? */
  {
     if (!VerfyPage())
       return ftag;
....
     return NULL;
  }
  char *mdfname(FileTag *ftag);
smgr.c:
  FileTag *smgrread(...);
  char *smgrfname(FileTag *ftag);

bufmgr.c:
  ReadBuffer_common()
  {
     FileTag *errtag;
  ..
            errftag = smgrread();

            if (errtag))
                /* page verification failed */
                if (mode == RBM_ZERO_ON_ERROR || zero_damaged_pages)
                    ereport(WARNING,
                             errmsg("invalid page in block %u in file %s; zeroing out page",
                                    blockNum, smgrfname(errftag)));
                    MemSet((char *) bufBlock, 0, BLCKSZ);
====

But it is uneasy that smgrread just returning a filetag signals
checksum error..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Feb 19, 2020 at 01:07:36PM +0900, Kyotaro Horiguchi wrote:
> If we also verify checksum in md layer, callback is overkill since the
> immediate caller consumes the event immediately.  We can signal the
> error by somehow returning a file tag.

FWIW, I am wondering if there is any need for a change here and
complicate more the code.  If you know the block number, the page size
and the segment file size you can immediately guess where is the
damaged block.  The first information is already part of the error
message, and the two other ones are constants defined at
compile-time.
--
Michael

At Wed, 19 Feb 2020 13:28:04 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Wed, Feb 19, 2020 at 01:07:36PM +0900, Kyotaro Horiguchi wrote:
> > If we also verify checksum in md layer, callback is overkill since the
> > immediate caller consumes the event immediately.  We can signal the
> > error by somehow returning a file tag.
> 
> FWIW, I am wondering if there is any need for a change here and
> complicate more the code.  If you know the block number, the page size
> and the segment file size you can immediately guess where is the
> damaged block.  The first information is already part of the error

I have had support requests related to broken block several times, and
(I think) most of *them* had hard time to locate the broken block or
even broken file.  I don't think it is useles at all, but I'm not sure
it is worth the additional complexity.

> damaged block.  The first information is already part of the error
> message, and the two other ones are constants defined at
> compile-time.

May you have misread the snippet?

What Hubert proposed is:

 "invalid page in block %u of relation file %s; zeroing out page",
    blkno, <filename>

The second format in my messages just before is:
  "invalid page in block %u in relation %u, file \"%s\"",
     blockNum, smgr->smgr_rnode.node.relNode, smgrfname()

All of them are not compile-time constant at all.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Feb 19, 2020 at 03:00:54PM +0900, Kyotaro Horiguchi wrote:
> I have had support requests related to broken block several times, and
> (I think) most of *them* had hard time to locate the broken block or
> even broken file.  I don't think it is useles at all, but I'm not sure
> it is worth the additional complexity.

I handle stuff like that from time to time, and any reports usually
go down to people knowledgeable about PostgreSQL enough to know the
difference.  My point is that in order to know where a broken block is
physically located on disk, you need to know four things:
- The block number.
- The physical location of the relation.
- The size of the block.
- The length of a file segment.
The first two items are printed in the error message, and you can
guess easily the actual location (file, offset) with the two others.

I am not necessarily against improving the error message here, but
FWIW I think that we need to consider seriously if the code
complications and the maintenance cost involved are really worth
saving from one simple calculation.  Particularly, quickly reading
through the patch, I am rather unhappy about the shape of the second
patch which pushes down the segment number knowledge into relpath.c,
and creates more complication around the handling of
zero_damaged_pages and zero'ed pages.
--
Michael

Hi,

On 2020-02-19 16:48:45 +0900, Michael Paquier wrote:
> On Wed, Feb 19, 2020 at 03:00:54PM +0900, Kyotaro Horiguchi wrote:
> > I have had support requests related to broken block several times, and
> > (I think) most of *them* had hard time to locate the broken block or
> > even broken file.  I don't think it is useles at all, but I'm not sure
> > it is worth the additional complexity.
>
> I handle stuff like that from time to time, and any reports usually
> go down to people knowledgeable about PostgreSQL enough to know the
> difference.  My point is that in order to know where a broken block is
> physically located on disk, you need to know four things:
> - The block number.
> - The physical location of the relation.
> - The size of the block.
> - The length of a file segment.
> The first two items are printed in the error message, and you can
> guess easily the actual location (file, offset) with the two others.

> I am not necessarily against improving the error message here, but
> FWIW I think that we need to consider seriously if the code
> complications and the maintenance cost involved are really worth
> saving from one simple calculation.

I don't think it's that simple for most.

And if we e.g. ever get the undo stuff merged, it'd get more
complicated, because they segment entirely differently. Similar, if we
ever manage to move SLRUs into the buffer pool and checksummed, it'd
again work differently.

Nor is it architecturally appealing to handle checksums in multiple
places above the smgr layer: For one, that requires multiple places to
compute verify them. But also, as the way checksums are computed depends
on the page format etc, it'll likely change for things like undo/slru -
which then again will require additional smarts if done above the smgr
layer.


> Particularly, quickly reading through the patch, I am rather unhappy
> about the shape of the second patch which pushes down the segment
> number knowledge into relpath.c, and creates more complication around
> the handling of zero_damaged_pages and zero'ed pages.  -- Michael

I do not like the SetZeroDamagedPageInChecksum stuff at all however.

Greetings,

Andres Freund