From e25fe65ef66e536ef6074a3f273c5cecd27f65b7 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Thu, 20 Feb 2025 15:12:28 -0500 Subject: [PATCH v2 1/2] Change relpath() etc to return path by value Discussion: https://postgr.es/m/xeri5mla4b5syjd5a25nok5iez2kr3bm26j2qn4u7okzof2bmf@kwdh2vf7npra --- src/include/common/relpath.h | 52 ++++++++++++- src/common/relpath.c | 77 ++++++++++--------- src/backend/access/rmgrdesc/smgrdesc.c | 10 +-- src/backend/access/rmgrdesc/xactdesc.c | 6 +- src/backend/access/transam/xlogutils.c | 29 +++---- src/backend/backup/basebackup_incremental.c | 10 +-- src/backend/catalog/catalog.c | 6 +- src/backend/catalog/storage.c | 4 +- .../replication/logical/reorderbuffer.c | 4 +- src/backend/storage/buffer/bufmgr.c | 48 +++++------- src/backend/storage/buffer/localbuf.c | 6 +- src/backend/storage/smgr/md.c | 59 ++++++-------- src/backend/utils/adt/dbsize.c | 10 +-- src/bin/pg_rewind/filemap.c | 7 +- src/test/regress/expected/misc_functions.out | 11 +++ src/test/regress/regress.c | 29 +++++++ src/test/regress/sql/misc_functions.sql | 8 ++ src/tools/pgindent/typedefs.list | 1 + 18 files changed, 218 insertions(+), 159 deletions(-) diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h index 5162362e7d8..72fe5ee77d5 100644 --- a/src/include/common/relpath.h +++ b/src/include/common/relpath.h @@ -77,13 +77,61 @@ extern PGDLLIMPORT const char *const forkNames[]; extern ForkNumber forkname_to_number(const char *forkName); extern int forkname_chars(const char *str, ForkNumber *fork); + +/* + * MAX_BACKENDS is 2^18-1, we don't want to include postmaster.h here and + * there's no obvious way to derive PROCNUMBER_CHARS from it + * anyway. Crosschecked in test_relpath(). + */ +#define PROCNUMBER_CHARS 6 + +/* + * The longest possible relation path lengths is from the following format: + * sprintf(rp.path, "%s/%u/%s/%u/t%d_%u", + * PG_TBLSPC_DIR, spcOid, + * TABLESPACE_VERSION_DIRECTORY, + * dbOid, procNumber, relNumber); + * + * Note this does *not* include the trailing null-byte, to make it easier to + * combine it with other lengths. + */ +#define REL_PATH_STR_MAXLEN \ + ( \ + sizeof(PG_TBLSPC_DIR) - 1 \ + + sizeof((char)'/') \ + + OIDCHARS /* spcOid */ \ + + sizeof((char)'/') \ + + sizeof(TABLESPACE_VERSION_DIRECTORY) - 1 \ + + sizeof((char)'/') \ + + OIDCHARS /* dbOid */ \ + + sizeof((char)'/') \ + + sizeof((char)'t') /* temporary table indicator */ \ + + PROCNUMBER_CHARS /* procNumber */ \ + + sizeof((char)'_') \ + + OIDCHARS /* relNumber */ \ + + sizeof((char)'_') \ + + FORKNAMECHARS /* forkNames[forkNumber] */ \ + ) + +/* + * String of the exact length required to represent a relation path. We return + * this struct, instead of char[REL_PATH_STR_MAXLEN + 1], as the pointer would + * decay to a plain char * too easily, possibly preventing the compiler from + * detecting invalid references to the on-stack return value of + * GetRelationPath(). + */ +typedef struct RelPathStr +{ + char str[REL_PATH_STR_MAXLEN + 1]; +} RelPathStr; + /* * Stuff for computing filesystem pathnames for relations. */ extern char *GetDatabasePath(Oid dbOid, Oid spcOid); -extern char *GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber, - int procNumber, ForkNumber forkNumber); +extern RelPathStr GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber, + int procNumber, ForkNumber forkNumber); /* * Wrapper macros for GetRelationPath. Beware of multiple diff --git a/src/common/relpath.c b/src/common/relpath.c index 186156a9e44..7dcf987afcd 100644 --- a/src/common/relpath.c +++ b/src/common/relpath.c @@ -132,17 +132,18 @@ GetDatabasePath(Oid dbOid, Oid spcOid) /* * GetRelationPath - construct path to a relation's file * - * Result is a palloc'd string. + * The result is returned in-place as a struct, to make it suitable for use in + * critical sections etc. * * Note: ideally, procNumber would be declared as type ProcNumber, but * relpath.h would have to include a backend-only header to do that; doesn't * seem worth the trouble considering ProcNumber is just int anyway. */ -char * +RelPathStr GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber, int procNumber, ForkNumber forkNumber) { - char *path; + RelPathStr rp; if (spcOid == GLOBALTABLESPACE_OID) { @@ -150,10 +151,11 @@ GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber, Assert(dbOid == 0); Assert(procNumber == INVALID_PROC_NUMBER); if (forkNumber != MAIN_FORKNUM) - path = psprintf("global/%u_%s", - relNumber, forkNames[forkNumber]); + sprintf(rp.str, "global/%u_%s", + relNumber, forkNames[forkNumber]); else - path = psprintf("global/%u", relNumber); + sprintf(rp.str, "global/%u", + relNumber); } else if (spcOid == DEFAULTTABLESPACE_OID) { @@ -161,22 +163,24 @@ GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber, if (procNumber == INVALID_PROC_NUMBER) { if (forkNumber != MAIN_FORKNUM) - path = psprintf("base/%u/%u_%s", - dbOid, relNumber, - forkNames[forkNumber]); + { + sprintf(rp.str, "base/%u/%u_%s", + dbOid, relNumber, + forkNames[forkNumber]); + } else - path = psprintf("base/%u/%u", - dbOid, relNumber); + sprintf(rp.str, "base/%u/%u", + dbOid, relNumber); } else { if (forkNumber != MAIN_FORKNUM) - path = psprintf("base/%u/t%d_%u_%s", - dbOid, procNumber, relNumber, - forkNames[forkNumber]); + sprintf(rp.str, "base/%u/t%d_%u_%s", + dbOid, procNumber, relNumber, + forkNames[forkNumber]); else - path = psprintf("base/%u/t%d_%u", - dbOid, procNumber, relNumber); + sprintf(rp.str, "base/%u/t%d_%u", + dbOid, procNumber, relNumber); } } else @@ -185,31 +189,34 @@ GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber, if (procNumber == INVALID_PROC_NUMBER) { if (forkNumber != MAIN_FORKNUM) - path = psprintf("%s/%u/%s/%u/%u_%s", - PG_TBLSPC_DIR, spcOid, - TABLESPACE_VERSION_DIRECTORY, - dbOid, relNumber, - forkNames[forkNumber]); + sprintf(rp.str, "%s/%u/%s/%u/%u_%s", + PG_TBLSPC_DIR, spcOid, + TABLESPACE_VERSION_DIRECTORY, + dbOid, relNumber, + forkNames[forkNumber]); else - path = psprintf("%s/%u/%s/%u/%u", - PG_TBLSPC_DIR, spcOid, - TABLESPACE_VERSION_DIRECTORY, - dbOid, relNumber); + sprintf(rp.str, "%s/%u/%s/%u/%u", + PG_TBLSPC_DIR, spcOid, + TABLESPACE_VERSION_DIRECTORY, + dbOid, relNumber); } else { if (forkNumber != MAIN_FORKNUM) - path = psprintf("%s/%u/%s/%u/t%d_%u_%s", - PG_TBLSPC_DIR, spcOid, - TABLESPACE_VERSION_DIRECTORY, - dbOid, procNumber, relNumber, - forkNames[forkNumber]); + sprintf(rp.str, "%s/%u/%s/%u/t%d_%u_%s", + PG_TBLSPC_DIR, spcOid, + TABLESPACE_VERSION_DIRECTORY, + dbOid, procNumber, relNumber, + forkNames[forkNumber]); else - path = psprintf("%s/%u/%s/%u/t%d_%u", - PG_TBLSPC_DIR, spcOid, - TABLESPACE_VERSION_DIRECTORY, - dbOid, procNumber, relNumber); + sprintf(rp.str, "%s/%u/%s/%u/t%d_%u", + PG_TBLSPC_DIR, spcOid, + TABLESPACE_VERSION_DIRECTORY, + dbOid, procNumber, relNumber); } } - return path; + + Assert(strnlen(rp.str, REL_PATH_STR_MAXLEN + 1) <= REL_PATH_STR_MAXLEN); + + return rp; } diff --git a/src/backend/access/rmgrdesc/smgrdesc.c b/src/backend/access/rmgrdesc/smgrdesc.c index 1cde5c2f1da..4bb7fc79bce 100644 --- a/src/backend/access/rmgrdesc/smgrdesc.c +++ b/src/backend/access/rmgrdesc/smgrdesc.c @@ -26,19 +26,17 @@ smgr_desc(StringInfo buf, XLogReaderState *record) if (info == XLOG_SMGR_CREATE) { xl_smgr_create *xlrec = (xl_smgr_create *) rec; - char *path = relpathperm(xlrec->rlocator, xlrec->forkNum); - appendStringInfoString(buf, path); - pfree(path); + appendStringInfoString(buf, + relpathperm(xlrec->rlocator, xlrec->forkNum).str); } else if (info == XLOG_SMGR_TRUNCATE) { xl_smgr_truncate *xlrec = (xl_smgr_truncate *) rec; - char *path = relpathperm(xlrec->rlocator, MAIN_FORKNUM); - appendStringInfo(buf, "%s to %u blocks flags %d", path, + appendStringInfo(buf, "%s to %u blocks flags %d", + relpathperm(xlrec->rlocator, MAIN_FORKNUM).str, xlrec->blkno, xlrec->flags); - pfree(path); } } diff --git a/src/backend/access/rmgrdesc/xactdesc.c b/src/backend/access/rmgrdesc/xactdesc.c index ec10f1c28c2..7f94810defc 100644 --- a/src/backend/access/rmgrdesc/xactdesc.c +++ b/src/backend/access/rmgrdesc/xactdesc.c @@ -287,10 +287,8 @@ xact_desc_relations(StringInfo buf, char *label, int nrels, appendStringInfo(buf, "; %s:", label); for (i = 0; i < nrels; i++) { - char *path = relpathperm(xlocators[i], MAIN_FORKNUM); - - appendStringInfo(buf, " %s", path); - pfree(path); + appendStringInfo(buf, " %s", + relpathperm(xlocators[i], MAIN_FORKNUM).str); } } } diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c index 68d53815925..b61a0eb4014 100644 --- a/src/backend/access/transam/xlogutils.c +++ b/src/backend/access/transam/xlogutils.c @@ -86,15 +86,14 @@ static void report_invalid_page(int elevel, RelFileLocator locator, ForkNumber forkno, BlockNumber blkno, bool present) { - char *path = relpathperm(locator, forkno); + RelPathStr path = relpathperm(locator, forkno); if (present) elog(elevel, "page %u of relation %s is uninitialized", - blkno, path); + blkno, path.str); else elog(elevel, "page %u of relation %s does not exist", - blkno, path); - pfree(path); + blkno, path.str); } /* Log a reference to an invalid page */ @@ -180,14 +179,9 @@ forget_invalid_pages(RelFileLocator locator, ForkNumber forkno, hentry->key.forkno == forkno && hentry->key.blkno >= minblkno) { - if (message_level_is_interesting(DEBUG2)) - { - char *path = relpathperm(hentry->key.locator, forkno); - - elog(DEBUG2, "page %u of relation %s has been dropped", - hentry->key.blkno, path); - pfree(path); - } + elog(DEBUG2, "page %u of relation %s has been dropped", + hentry->key.blkno, + relpathperm(hentry->key.locator, forkno).str); if (hash_search(invalid_page_tab, &hentry->key, @@ -213,14 +207,9 @@ forget_invalid_pages_db(Oid dbid) { if (hentry->key.locator.dbOid == dbid) { - if (message_level_is_interesting(DEBUG2)) - { - char *path = relpathperm(hentry->key.locator, hentry->key.forkno); - - elog(DEBUG2, "page %u of relation %s has been dropped", - hentry->key.blkno, path); - pfree(path); - } + elog(DEBUG2, "page %u of relation %s has been dropped", + hentry->key.blkno, + relpathperm(hentry->key.locator, hentry->key.forkno).str); if (hash_search(invalid_page_tab, &hentry->key, diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c index 360711fadb8..c2b7a55e347 100644 --- a/src/backend/backup/basebackup_incremental.c +++ b/src/backend/backup/basebackup_incremental.c @@ -625,23 +625,21 @@ char * GetIncrementalFilePath(Oid dboid, Oid spcoid, RelFileNumber relfilenumber, ForkNumber forknum, unsigned segno) { - char *path; + RelPathStr path; char *lastslash; char *ipath; path = GetRelationPath(dboid, spcoid, relfilenumber, INVALID_PROC_NUMBER, forknum); - lastslash = strrchr(path, '/'); + lastslash = strrchr(path.str, '/'); Assert(lastslash != NULL); *lastslash = '\0'; if (segno > 0) - ipath = psprintf("%s/INCREMENTAL.%s.%u", path, lastslash + 1, segno); + ipath = psprintf("%s/INCREMENTAL.%s.%u", path.str, lastslash + 1, segno); else - ipath = psprintf("%s/INCREMENTAL.%s", path, lastslash + 1); - - pfree(path); + ipath = psprintf("%s/INCREMENTAL.%s", path.str, lastslash + 1); return ipath; } diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c index 05ebe248f12..8436e312d4d 100644 --- a/src/backend/catalog/catalog.c +++ b/src/backend/catalog/catalog.c @@ -528,7 +528,7 @@ RelFileNumber GetNewRelFileNumber(Oid reltablespace, Relation pg_class, char relpersistence) { RelFileLocatorBackend rlocator; - char *rpath; + RelPathStr rpath; bool collides; ProcNumber procNumber; @@ -580,7 +580,7 @@ GetNewRelFileNumber(Oid reltablespace, Relation pg_class, char relpersistence) /* Check for existing file of same name */ rpath = relpath(rlocator, MAIN_FORKNUM); - if (access(rpath, F_OK) == 0) + if (access(rpath.str, F_OK) == 0) { /* definite collision */ collides = true; @@ -596,8 +596,6 @@ GetNewRelFileNumber(Oid reltablespace, Relation pg_class, char relpersistence) */ collides = false; } - - pfree(rpath); } while (collides); return rlocator.locator.relNumber; diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c index 690b1dbc6ee..624ed41bbf3 100644 --- a/src/backend/catalog/storage.c +++ b/src/backend/catalog/storage.c @@ -524,14 +524,14 @@ RelationCopyStorage(SMgrRelation src, SMgrRelation dst, * (errcontext callbacks shouldn't be risking any such thing, but * people have been known to forget that rule.) */ - char *relpath = relpathbackend(src->smgr_rlocator.locator, + RelPathStr relpath = relpathbackend(src->smgr_rlocator.locator, src->smgr_rlocator.backend, forkNum); ereport(ERROR, (errcode(ERRCODE_DATA_CORRUPTED), errmsg("invalid page in block %u of relation %s", - blkno, relpath))); + blkno, relpath.str))); } /* diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index b42f4002ba8..5186ad2a397 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -2329,7 +2329,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, else if (reloid == InvalidOid) elog(ERROR, "could not map filenumber \"%s\" to relation OID", relpathperm(change->data.tp.rlocator, - MAIN_FORKNUM)); + MAIN_FORKNUM).str); relation = RelationIdGetRelation(reloid); @@ -2337,7 +2337,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, elog(ERROR, "could not open relation with OID %u (for filenumber \"%s\")", reloid, relpathperm(change->data.tp.rlocator, - MAIN_FORKNUM)); + MAIN_FORKNUM).str); if (!RelationIsLogicallyLogged(relation)) goto change_done; diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 75cfc2b6fe9..7915ed624c1 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -1541,7 +1541,7 @@ WaitReadBuffers(ReadBuffersOperation *operation) (errcode(ERRCODE_DATA_CORRUPTED), errmsg("invalid page in block %u of relation %s; zeroing out page", io_first_block + j, - relpath(operation->smgr->smgr_rlocator, forknum)))); + relpath(operation->smgr->smgr_rlocator, forknum).str))); memset(bufBlock, 0, BLCKSZ); } else @@ -1549,7 +1549,7 @@ WaitReadBuffers(ReadBuffersOperation *operation) (errcode(ERRCODE_DATA_CORRUPTED), errmsg("invalid page in block %u of relation %s", io_first_block + j, - relpath(operation->smgr->smgr_rlocator, forknum)))); + relpath(operation->smgr->smgr_rlocator, forknum).str))); } /* Terminate I/O and set BM_VALID. */ @@ -2284,7 +2284,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr, ereport(ERROR, (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("cannot extend relation %s beyond %u blocks", - relpath(bmr.smgr->smgr_rlocator, fork), + relpath(bmr.smgr->smgr_rlocator, fork).str, MaxBlockNumber))); /* @@ -2355,7 +2355,8 @@ ExtendBufferedRelShared(BufferManagerRelation bmr, if (valid && !PageIsNew((Page) buf_block)) ereport(ERROR, (errmsg("unexpected data beyond EOF in block %u of relation %s", - existing_hdr->tag.blockNum, relpath(bmr.smgr->smgr_rlocator, fork)), + existing_hdr->tag.blockNum, + relpath(bmr.smgr->smgr_rlocator, fork).str), errhint("This has been seen to occur with buggy kernels; consider updating your system."))); /* @@ -3663,7 +3664,6 @@ DebugPrintBufferRefcount(Buffer buffer) { BufferDesc *buf; int32 loccount; - char *path; char *result; ProcNumber backend; uint32 buf_state; @@ -3683,15 +3683,14 @@ DebugPrintBufferRefcount(Buffer buffer) } /* theoretically we should lock the bufhdr here */ - path = relpathbackend(BufTagGetRelFileLocator(&buf->tag), backend, - BufTagGetForkNum(&buf->tag)); buf_state = pg_atomic_read_u32(&buf->state); result = psprintf("[%03d] (rel=%s, blockNum=%u, flags=0x%x, refcount=%u %d)", - buffer, path, + buffer, + relpathbackend(BufTagGetRelFileLocator(&buf->tag), backend, + BufTagGetForkNum(&buf->tag)).str, buf->tag.blockNum, buf_state & BUF_FLAG_MASK, BUF_STATE_GET_REFCOUNT(buf_state), loccount); - pfree(path); return result; } @@ -5611,16 +5610,13 @@ AbortBufferIO(Buffer buffer) if (buf_state & BM_IO_ERROR) { /* Buffer is pinned, so we can read tag without spinlock */ - char *path; - - path = relpathperm(BufTagGetRelFileLocator(&buf_hdr->tag), - BufTagGetForkNum(&buf_hdr->tag)); ereport(WARNING, (errcode(ERRCODE_IO_ERROR), errmsg("could not write block %u of %s", - buf_hdr->tag.blockNum, path), + buf_hdr->tag.blockNum, + relpathperm(BufTagGetRelFileLocator(&buf_hdr->tag), + BufTagGetForkNum(&buf_hdr->tag)).str), errdetail("Multiple failures --- write error might be permanent."))); - pfree(path); } } @@ -5637,14 +5633,10 @@ shared_buffer_write_error_callback(void *arg) /* Buffer is pinned, so we can read the tag without locking the spinlock */ if (bufHdr != NULL) - { - char *path = relpathperm(BufTagGetRelFileLocator(&bufHdr->tag), - BufTagGetForkNum(&bufHdr->tag)); - errcontext("writing block %u of relation %s", - bufHdr->tag.blockNum, path); - pfree(path); - } + bufHdr->tag.blockNum, + relpathperm(BufTagGetRelFileLocator(&bufHdr->tag), + BufTagGetForkNum(&bufHdr->tag)).str); } /* @@ -5656,15 +5648,11 @@ local_buffer_write_error_callback(void *arg) BufferDesc *bufHdr = (BufferDesc *) arg; if (bufHdr != NULL) - { - char *path = relpathbackend(BufTagGetRelFileLocator(&bufHdr->tag), - MyProcNumber, - BufTagGetForkNum(&bufHdr->tag)); - errcontext("writing block %u of relation %s", - bufHdr->tag.blockNum, path); - pfree(path); - } + bufHdr->tag.blockNum, + relpathbackend(BufTagGetRelFileLocator(&bufHdr->tag), + MyProcNumber, + BufTagGetForkNum(&bufHdr->tag)).str); } /* diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c index 64931efaa75..80b83444eb2 100644 --- a/src/backend/storage/buffer/localbuf.c +++ b/src/backend/storage/buffer/localbuf.c @@ -360,7 +360,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr, ereport(ERROR, (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("cannot extend relation %s beyond %u blocks", - relpath(bmr.smgr->smgr_rlocator, fork), + relpath(bmr.smgr->smgr_rlocator, fork).str, MaxBlockNumber))); for (uint32 i = 0; i < extend_by; i++) @@ -510,7 +510,7 @@ DropRelationLocalBuffers(RelFileLocator rlocator, ForkNumber forkNum, bufHdr->tag.blockNum, relpathbackend(BufTagGetRelFileLocator(&bufHdr->tag), MyProcNumber, - BufTagGetForkNum(&bufHdr->tag)), + BufTagGetForkNum(&bufHdr->tag)).str, LocalRefCount[i]); /* Remove entry from hashtable */ @@ -555,7 +555,7 @@ DropRelationAllLocalBuffers(RelFileLocator rlocator) bufHdr->tag.blockNum, relpathbackend(BufTagGetRelFileLocator(&bufHdr->tag), MyProcNumber, - BufTagGetForkNum(&bufHdr->tag)), + BufTagGetForkNum(&bufHdr->tag)).str, LocalRefCount[i]); /* Remove entry from hashtable */ hresult = (LocalBufferLookupEnt *) diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c index 7bf0b45e2c3..157876967d1 100644 --- a/src/backend/storage/smgr/md.c +++ b/src/backend/storage/smgr/md.c @@ -182,7 +182,7 @@ void mdcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo) { MdfdVec *mdfd; - char *path; + RelPathStr path; File fd; if (isRedo && reln->md_num_open_segs[forknum] > 0) @@ -205,26 +205,24 @@ mdcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo) path = relpath(reln->smgr_rlocator, forknum); - fd = PathNameOpenFile(path, _mdfd_open_flags() | O_CREAT | O_EXCL); + fd = PathNameOpenFile(path.str, _mdfd_open_flags() | O_CREAT | O_EXCL); if (fd < 0) { int save_errno = errno; if (isRedo) - fd = PathNameOpenFile(path, _mdfd_open_flags()); + fd = PathNameOpenFile(path.str, _mdfd_open_flags()); if (fd < 0) { /* be sure to report the error reported by create, not open */ errno = save_errno; ereport(ERROR, (errcode_for_file_access(), - errmsg("could not create file \"%s\": %m", path))); + errmsg("could not create file \"%s\": %m", path.str))); } } - pfree(path); - _fdvec_resize(reln, forknum, 1); mdfd = &reln->md_seg_fds[forknum][0]; mdfd->mdfd_vfd = fd; @@ -335,7 +333,7 @@ do_truncate(const char *path) static void mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo) { - char *path; + RelPathStr path; int ret; int save_errno; @@ -351,7 +349,7 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo) if (!RelFileLocatorBackendIsTemp(rlocator)) { /* Prevent other backends' fds from holding on to the disk space */ - ret = do_truncate(path); + ret = do_truncate(path.str); /* Forget any pending sync requests for the first segment */ save_errno = errno; @@ -364,13 +362,13 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo) /* Next unlink the file, unless it was already found to be missing */ if (ret >= 0 || errno != ENOENT) { - ret = unlink(path); + ret = unlink(path.str); if (ret < 0 && errno != ENOENT) { save_errno = errno; ereport(WARNING, (errcode_for_file_access(), - errmsg("could not remove file \"%s\": %m", path))); + errmsg("could not remove file \"%s\": %m", path.str))); errno = save_errno; } } @@ -378,7 +376,7 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo) else { /* Prevent other backends' fds from holding on to the disk space */ - ret = do_truncate(path); + ret = do_truncate(path.str); /* Register request to unlink first segment later */ save_errno = errno; @@ -400,12 +398,12 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo) */ if (ret >= 0 || errno != ENOENT) { - char *segpath = (char *) palloc(strlen(path) + 12); + char *segpath = (char *) palloc(strlen(path.str) + 12); BlockNumber segno; for (segno = 1;; segno++) { - sprintf(segpath, "%s.%u", path, segno); + sprintf(segpath, "%s.%u", path.str, segno); if (!RelFileLocatorBackendIsTemp(rlocator)) { @@ -435,8 +433,6 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo) } pfree(segpath); } - - pfree(path); } /* @@ -475,7 +471,7 @@ mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, ereport(ERROR, (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("cannot extend file \"%s\" beyond %u blocks", - relpath(reln->smgr_rlocator, forknum), + relpath(reln->smgr_rlocator, forknum).str, InvalidBlockNumber))); v = _mdfd_getseg(reln, forknum, blocknum, skipFsync, EXTENSION_CREATE); @@ -537,7 +533,7 @@ mdzeroextend(SMgrRelation reln, ForkNumber forknum, ereport(ERROR, (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("cannot extend file \"%s\" beyond %u blocks", - relpath(reln->smgr_rlocator, forknum), + relpath(reln->smgr_rlocator, forknum).str, InvalidBlockNumber))); while (remblocks > 0) @@ -629,7 +625,7 @@ static MdfdVec * mdopenfork(SMgrRelation reln, ForkNumber forknum, int behavior) { MdfdVec *mdfd; - char *path; + RelPathStr path; File fd; /* No work if already open */ @@ -638,23 +634,18 @@ mdopenfork(SMgrRelation reln, ForkNumber forknum, int behavior) path = relpath(reln->smgr_rlocator, forknum); - fd = PathNameOpenFile(path, _mdfd_open_flags()); + fd = PathNameOpenFile(path.str, _mdfd_open_flags()); if (fd < 0) { if ((behavior & EXTENSION_RETURN_NULL) && FILE_POSSIBLY_DELETED(errno)) - { - pfree(path); return NULL; - } ereport(ERROR, (errcode_for_file_access(), - errmsg("could not open file \"%s\": %m", path))); + errmsg("could not open file \"%s\": %m", path.str))); } - pfree(path); - _fdvec_resize(reln, forknum, 1); mdfd = &reln->md_seg_fds[forknum][0]; mdfd->mdfd_vfd = fd; @@ -1176,7 +1167,7 @@ mdtruncate(SMgrRelation reln, ForkNumber forknum, return; ereport(ERROR, (errmsg("could not truncate file \"%s\" to %u blocks: it's only %u blocks now", - relpath(reln->smgr_rlocator, forknum), + relpath(reln->smgr_rlocator, forknum).str, nblocks, curnblk))); } if (nblocks == curnblk) @@ -1540,18 +1531,15 @@ _fdvec_resize(SMgrRelation reln, static char * _mdfd_segpath(SMgrRelation reln, ForkNumber forknum, BlockNumber segno) { - char *path, - *fullpath; + RelPathStr path; + char *fullpath; path = relpath(reln->smgr_rlocator, forknum); if (segno > 0) - { - fullpath = psprintf("%s.%u", path, segno); - pfree(path); - } + fullpath = psprintf("%s.%u", path.str, segno); else - fullpath = path; + fullpath = pstrdup(path.str); return fullpath; } @@ -1811,12 +1799,11 @@ mdsyncfiletag(const FileTag *ftag, char *path) int mdunlinkfiletag(const FileTag *ftag, char *path) { - char *p; + RelPathStr p; /* Compute the path. */ p = relpathperm(ftag->rlocator, MAIN_FORKNUM); - strlcpy(path, p, MAXPGPATH); - pfree(p); + strlcpy(path, p.str, MAXPGPATH); /* Try to unlink the file. */ return unlink(path); diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c index 011d8d4da5a..25865b660ef 100644 --- a/src/backend/utils/adt/dbsize.c +++ b/src/backend/utils/adt/dbsize.c @@ -326,7 +326,7 @@ static int64 calculate_relation_size(RelFileLocator *rfn, ProcNumber backend, ForkNumber forknum) { int64 totalsize = 0; - char *relationpath; + RelPathStr relationpath; char pathname[MAXPGPATH]; unsigned int segcount = 0; @@ -340,10 +340,10 @@ calculate_relation_size(RelFileLocator *rfn, ProcNumber backend, ForkNumber fork if (segcount == 0) snprintf(pathname, MAXPGPATH, "%s", - relationpath); + relationpath.str); else snprintf(pathname, MAXPGPATH, "%s.%u", - relationpath, segcount); + relationpath.str, segcount); if (stat(pathname, &fst) < 0) { @@ -973,7 +973,7 @@ pg_relation_filepath(PG_FUNCTION_ARGS) Form_pg_class relform; RelFileLocator rlocator; ProcNumber backend; - char *path; + RelPathStr path; tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); if (!HeapTupleIsValid(tuple)) @@ -1039,5 +1039,5 @@ pg_relation_filepath(PG_FUNCTION_ARGS) path = relpathbackend(rlocator, backend, MAIN_FORKNUM); - PG_RETURN_TEXT_P(cstring_to_text(path)); + PG_RETURN_TEXT_P(cstring_to_text(path.str)); } diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c index f1beb371203..a28d1667d4c 100644 --- a/src/bin/pg_rewind/filemap.c +++ b/src/bin/pg_rewind/filemap.c @@ -652,18 +652,17 @@ isRelDataFile(const char *path) static char * datasegpath(RelFileLocator rlocator, ForkNumber forknum, BlockNumber segno) { - char *path; + RelPathStr path; char *segpath; path = relpathperm(rlocator, forknum); if (segno > 0) { - segpath = psprintf("%s.%u", path, segno); - pfree(path); + segpath = psprintf("%s.%u", path.str, segno); return segpath; } else - return path; + return pstrdup(path.str); } /* diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index 106dedb519a..543fbbe09c5 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -903,3 +903,14 @@ SELECT gist_stratnum_common(3); 18 (1 row) +-- relpath tests +CREATE FUNCTION test_relpath() + RETURNS void + AS :'regresslib' + LANGUAGE C; +SELECT test_relpath(); + test_relpath +-------------- + +(1 row) + diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index 667d9835148..ed4a7937331 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -36,6 +36,7 @@ #include "optimizer/plancat.h" #include "parser/parse_coerce.h" #include "port/atomics.h" +#include "postmaster/postmaster.h" /* for MAX_BACKENDS */ #include "storage/spin.h" #include "utils/array.h" #include "utils/builtins.h" @@ -1208,3 +1209,31 @@ binary_coercible(PG_FUNCTION_ARGS) PG_RETURN_BOOL(IsBinaryCoercible(srctype, targettype)); } + +/* + * Sanity checks for functions in relpath.h + */ +PG_FUNCTION_INFO_V1(test_relpath); +Datum +test_relpath(PG_FUNCTION_ARGS) +{ + RelPathStr rpath; + + /* + * Verify that PROCNUMBER_CHARS and MAX_BACKENDS stay in sync. + * Unfortunately I don't know how to express that in a way suitable for a + * static assert. + */ + if ((int) ceil(log10(MAX_BACKENDS)) != PROCNUMBER_CHARS) + elog(WARNING, "mismatch between MAX_BACKENDS and PROCNUMBER_CHARS"); + + /* verify that the max-length relpath is generated ok */ + rpath = GetRelationPath(OID_MAX, OID_MAX, OID_MAX, MAX_BACKENDS - 1, + INIT_FORKNUM); + + if (strlen(rpath.str) != REL_PATH_STR_MAXLEN) + elog(WARNING, "maximum length relpath is if length %zu instead of %zu", + strlen(rpath.str), REL_PATH_STR_MAXLEN); + + PG_RETURN_VOID(); +} diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index 753a0f41c03..aaebb298330 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -403,3 +403,11 @@ DROP FUNCTION explain_mask_costs(text, bool, bool, bool, bool); -- test stratnum support functions SELECT gist_stratnum_common(7); SELECT gist_stratnum_common(3); + + +-- relpath tests +CREATE FUNCTION test_relpath() + RETURNS void + AS :'regresslib' + LANGUAGE C; +SELECT test_relpath(); diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index b09d8af7183..a44b62ca59d 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2411,6 +2411,7 @@ RelMapFile RelMapping RelOptInfo RelOptKind +RelPathStr RelStatsInfo RelToCheck RelToCluster -- 2.48.1.76.g4e746b1a31.dirty