Change relpath() et al to return path by value
authorAndres Freund <andres@anarazel.de>
Tue, 25 Feb 2025 14:02:07 +0000 (09:02 -0500)
committerAndres Freund <andres@anarazel.de>
Tue, 25 Feb 2025 14:02:07 +0000 (09:02 -0500)
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

18 files changed:
src/backend/access/rmgrdesc/smgrdesc.c
src/backend/access/rmgrdesc/xactdesc.c
src/backend/access/transam/xlogutils.c
src/backend/backup/basebackup_incremental.c
src/backend/catalog/catalog.c
src/backend/catalog/storage.c
src/backend/replication/logical/reorderbuffer.c
src/backend/storage/buffer/bufmgr.c
src/backend/storage/buffer/localbuf.c
src/backend/storage/smgr/md.c
src/backend/utils/adt/dbsize.c
src/bin/pg_rewind/filemap.c
src/common/relpath.c
src/include/common/relpath.h
src/test/regress/expected/misc_functions.out
src/test/regress/regress.c
src/test/regress/sql/misc_functions.sql
src/tools/pgindent/typedefs.list

index 1cde5c2f1dadb91c5348a38f2bf05801c140ebd8..4bb7fc79bced60ba66b3bb7fb4575817101511d9 100644 (file)
@@ -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);
    }
 }
 
index ec10f1c28c2f9d9c8322912da2b522938819db74..7f94810defc32d9403ad2de582d645c26b5c614e 100644 (file)
@@ -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);
        }
    }
 }
index 68d5381592587a6114e8c5461fd5317af80e47dd..b61a0eb401446f7de6391b28ff8d4dd359656d4a 100644 (file)
@@ -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,
index 360711fadb87bec7740409eb6fbc623e0d36a05e..c2b7a55e347d12cf5bb6161e6283b8bb04608771 100644 (file)
@@ -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;
 }
index 05ebe248f1211ee63c72563b5bc934960b453733..8436e312d4d2fa978c9789d3aaa716461a92c1a9 100644 (file)
@@ -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;
index 690b1dbc6eed401a005cabf4522bebaa84c6896b..624ed41bbf3aa047304ca996e5986f030de20250 100644 (file)
@@ -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)));
        }
 
        /*
index b42f4002ba84589bc4efd6d58fe06d6dcba0735f..5186ad2a39703c12dbdd10662cb39e071b13f485 100644 (file)
@@ -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;
index d3216ef6ba758c9bf9cd1ad61eafdba7560461e0..40e9efec3128ae0bfe5f2f7f282d1e310dfba673 100644 (file)
@@ -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);
 }
 
 /*
index 78b65fb160adea1a3a360ba77963ae4da3f1ebce..d2f8158d69798b16b81e994595c528016a3d3653 100644 (file)
@@ -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 *)
index 298754d1b64a75e3f6aa0323048ca1edeccc6080..a570a0a909248d1d842ea6b97bc4f966d39db2b9 100644 (file)
@@ -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);
index 011d8d4da5aaaee8a310694c1cbaad0d5ee2553e..25865b660ef83d18d3f7946389f6213edcb655db 100644 (file)
@@ -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));
 }
index f1beb37120325357f37647899394705eee7894e9..a28d1667d4c8c5696a7cd183635b090bd5e1ca9b 100644 (file)
@@ -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);
 }
 
 /*
index 186156a9e44141db5ef8694e23fcb97f8ca6ac9a..7dcf987afcd01a86d211b3b4020abe2f0b3a2ea7 100644 (file)
@@ -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;
 }
index 5162362e7d8bddce1b7752d1a43363005cfb8610..6f2fce216f8c578572ee94bd97ac5857e7a2c74e 100644 (file)
@@ -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
index 106dedb519abeb755e66e25459320dbc20eced46..543fbbe09c58d635eef44d2c476663b7556ee91e 100644 (file)
@@ -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)
+
index 667d9835148f11343e5e0f6f253001d24a5d7b13..ed4a7937331ef6d11eee7c59ac3e820f7b41b676 100644 (file)
@@ -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();
+}
index 753a0f41c033d997a72c7fb6aa1470dc7a9c8eba..aaebb298330b8520d448ae6afa14179abeb4ca16 100644 (file)
@@ -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();
index e3e09a2207e97281c9dc4413bf5676c9137448a8..4200cf2fee8987ae808c4b46614723cd02422b94 100644 (file)
@@ -2410,6 +2410,7 @@ RelMapFile
 RelMapping
 RelOptInfo
 RelOptKind
+RelPathStr
 RelStatsInfo
 RelToCheck
 RelToCluster