Hi, On 2025-02-20 15:28:39 -0500, Andres Freund wrote: > On 2025-02-20 14:11:16 -0500, Tom Lane wrote: > > As a matter of style, I wonder if it'd be better to have these > > functions write into a caller-supplied variable. > > I wondered about that too, it does however make some code more awkward, > e.g. because there's not a good surrounding block to put the variable in. > > I mildly prefer the return-by-value approach, but not more.
Since I've not heard anything on that point, I'm currently thinking with going with return-by-value. Attached are two very mildly updated patches. Most of the changes is added commit messages. Greetings, Andres Freund
>From a94e33869aac2fb8e961eb25e7dd75261339f241 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Thu, 20 Feb 2025 15:12:28 -0500 Subject: [PATCH v3 1/2] Change relpath() et al to return path by value For AIO, and also some other recent patches, we need the ability to call relpath() in a critical section. Until now that was not feasible, as it allocated memory. The fact that relpath() allocated memory also made it awkward to use in log messages because we had to take care to free the memory afterwards. Which we e.g. didn't do for when zeroing out an invalid buffer. We discussed other solutions, e.g. filling a pre-allocated buffer that's passed to relpath(), but they all came with plenty downsides or were larger projects. The easiest fix seems to be to make relpath() return the path by value. To be able to return the path by value we need to determine the maximum length of a relation path. This patch adds a long #define that computes the exact maximum, which is verified to be correct in a regression test. As this change the signature of relpath(), extensions using it will need to adapt their code. We discussed leaving a backward-compat shim in place, but decided it's not worth it given the use of relpath() doesn't seem widespread. 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..6f2fce216f8 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); + +/* + * Unfortunately, there's no easy way to derive PROCNUMBER_CHARS from + * MAX_BACKENDS. MAX_BACKENDS is 2^18-1. 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 d3216ef6ba7..40e9efec312 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 78b65fb160a..d2f8158d697 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 298754d1b64..a570a0a9092 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 e3e09a2207e..4200cf2fee8 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2410,6 +2410,7 @@ RelMapFile RelMapping RelOptInfo RelOptKind +RelPathStr RelStatsInfo RelToCheck RelToCluster -- 2.46.0.519.g2e7b89e038
>From bce2daeac10134c588a787585461f0ae16519491 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Thu, 20 Feb 2025 15:15:07 -0500 Subject: [PATCH v3 2/2] Change _mdfd_segpath() to return paths by value This basically mirrors the changes done in the predecessor commit. While there isn't currently a need to get these paths in critical sections, it seems a shame to unnecessarily allocate memory in these paths now that relpath() doesn't allocate anymore. Discussion: https://postgr.es/m/xeri5mla4b5syjd5a25nok5iez2kr3bm26j2qn4u7okzof2bmf@kwdh2vf7npra --- src/backend/storage/smgr/md.c | 58 ++++++++++++++++++++------------ src/tools/pgindent/typedefs.list | 1 + 2 files changed, 38 insertions(+), 21 deletions(-) diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c index a570a0a9092..4994c97795c 100644 --- a/src/backend/storage/smgr/md.c +++ b/src/backend/storage/smgr/md.c @@ -110,6 +110,26 @@ static MemoryContext MdCxt; /* context for all MdfdVec objects */ #define EXTENSION_DONT_OPEN (1 << 5) +/* + * Fixed-length string to represent paths to files that need to be built by + * md.c. + * + * The maximum number of segments is MaxBlockNumber / RELSEG_SIZE, where + * RELSEG_SIZE can be set to 1 (for testing only). + */ +#define SEGMENT_CHARS OIDCHARS +#define MD_PATH_STR_MAXLEN \ + (\ + REL_PATH_STR_MAXLEN \ + + sizeof((char)'.') \ + + SEGMENT_CHARS \ + ) +typedef struct MdPathStr +{ + char str[MD_PATH_STR_MAXLEN + 1]; +} MdPathStr; + + /* local routines */ static void mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo); @@ -123,8 +143,8 @@ static void register_forget_request(RelFileLocatorBackend rlocator, ForkNumber f static void _fdvec_resize(SMgrRelation reln, ForkNumber forknum, int nseg); -static char *_mdfd_segpath(SMgrRelation reln, ForkNumber forknum, - BlockNumber segno); +static MdPathStr _mdfd_segpath(SMgrRelation reln, ForkNumber forknum, + BlockNumber segno); static MdfdVec *_mdfd_openseg(SMgrRelation reln, ForkNumber forknum, BlockNumber segno, int oflags); static MdfdVec *_mdfd_getseg(SMgrRelation reln, ForkNumber forknum, @@ -398,12 +418,12 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo) */ if (ret >= 0 || errno != ENOENT) { - char *segpath = (char *) palloc(strlen(path.str) + 12); + MdPathStr segpath; BlockNumber segno; for (segno = 1;; segno++) { - sprintf(segpath, "%s.%u", path.str, segno); + sprintf(segpath.str, "%s.%u", path.str, segno); if (!RelFileLocatorBackendIsTemp(rlocator)) { @@ -411,7 +431,7 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo) * Prevent other backends' fds from holding on to the disk * space. We're done if we see ENOENT, though. */ - if (do_truncate(segpath) < 0 && errno == ENOENT) + if (do_truncate(segpath.str) < 0 && errno == ENOENT) break; /* @@ -421,17 +441,16 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo) register_forget_request(rlocator, forknum, segno); } - if (unlink(segpath) < 0) + if (unlink(segpath.str) < 0) { /* ENOENT is expected after the last segment... */ if (errno != ENOENT) ereport(WARNING, (errcode_for_file_access(), - errmsg("could not remove file \"%s\": %m", segpath))); + errmsg("could not remove file \"%s\": %m", segpath.str))); break; } } - pfree(segpath); } } @@ -1528,18 +1547,18 @@ _fdvec_resize(SMgrRelation reln, * Return the filename for the specified segment of the relation. The * returned string is palloc'd. */ -static char * +static MdPathStr _mdfd_segpath(SMgrRelation reln, ForkNumber forknum, BlockNumber segno) { RelPathStr path; - char *fullpath; + MdPathStr fullpath; path = relpath(reln->smgr_rlocator, forknum); if (segno > 0) - fullpath = psprintf("%s.%u", path.str, segno); + sprintf(fullpath.str, "%s.%u", path.str, segno); else - fullpath = pstrdup(path.str); + strcpy(fullpath.str, path.str); return fullpath; } @@ -1554,14 +1573,12 @@ _mdfd_openseg(SMgrRelation reln, ForkNumber forknum, BlockNumber segno, { MdfdVec *v; File fd; - char *fullpath; + MdPathStr fullpath; fullpath = _mdfd_segpath(reln, forknum, segno); /* open the file */ - fd = PathNameOpenFile(fullpath, _mdfd_open_flags() | oflags); - - pfree(fullpath); + fd = PathNameOpenFile(fullpath.str, _mdfd_open_flags() | oflags); if (fd < 0) return NULL; @@ -1697,7 +1714,7 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno, ereport(ERROR, (errcode_for_file_access(), errmsg("could not open file \"%s\" (target block %u): previous segment is only %u blocks", - _mdfd_segpath(reln, forknum, nextsegno), + _mdfd_segpath(reln, forknum, nextsegno).str, blkno, nblocks))); } @@ -1711,7 +1728,7 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno, ereport(ERROR, (errcode_for_file_access(), errmsg("could not open file \"%s\" (target block %u): %m", - _mdfd_segpath(reln, forknum, nextsegno), + _mdfd_segpath(reln, forknum, nextsegno).str, blkno))); } } @@ -1762,11 +1779,10 @@ mdsyncfiletag(const FileTag *ftag, char *path) } else { - char *p; + MdPathStr p; p = _mdfd_segpath(reln, ftag->forknum, ftag->segno); - strlcpy(path, p, MAXPGPATH); - pfree(p); + strlcpy(path, p.str, MD_PATH_STR_MAXLEN); file = PathNameOpenFile(path, _mdfd_open_flags()); if (file < 0) diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 4200cf2fee8..1649f5e1011 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -1623,6 +1623,7 @@ Material MaterialPath MaterialState MdfdVec +MdPathStr Memoize MemoizeEntry MemoizeInstrumentation -- 2.46.0.519.g2e7b89e038
