Re: 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