Hi, 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. > That seems more in keeping with most other places in Postgres, and it would > save a copying step in cases where the caller needs the result on the heap. I don't think there are many cases where we have to then copy the value to the heap, fwiw, with a more complete patch. It's perhaps worth noting that on most (all?) architectures the *caller* will reserve space for the return value of functions that return structs by value. In general it'll be easier for the compiler to optimize code where it knows the lifetime of the relevant memory, which is harder for a function that gets passed a "target" memory location than when it's returning by value. But of course it's very hard to believe this ever matters for the case at hand. The patch curently uses a hardcoded 6 for the length of MAX_BACKENDS. Does anybody have a good idea for how to either a) derive 6 from MAX_BACKENDS in a way that can be used in a C array size b) Use a static assert to check that it fits? Otherwise I'll just put it in the test that my prior version already added. Not as good as compile-time, but should still make it easy to find the issue, if we ever increase/decrease MAX_BACKENDS. In the attached I 1) changed all the GetRelationPath()/relpath* uses to the new way, the names now are unchanged 2) introduced a PROCNUMBER_CHARS, instead hardcoding 6 in the length 3) renamed RelPathString to RelPathStr and change the path member to str I also added a second patch changing _mdfd_segpath() to do something similar, because it felt somewhat silly to allocate memory after the prior change. Not entirely sure it's worth it. Greetings, Andres Freund
>From e25fe65ef66e536ef6074a3f273c5cecd27f65b7 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> 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
>From ee29373d7be39bac811c0dc2edb62915e916b151 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Thu, 20 Feb 2025 15:15:07 -0500 Subject: [PATCH v2 2/2] Change _mdfd_segpath() to return paths by value 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 157876967d1..f3220f98dc4 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 a44b62ca59d..f47ea539dec 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.48.1.76.g4e746b1a31.dirty
