GetRelationPath() vs critical sections

Started by Andres Freundover 1 year ago18 messages

andres@anarazel.de

over 1 year ago

Hi,

In the AIO patchset there are cases where we have to LOG failures inside a
critical section. This is necessary because e.g. a buffer read might complete
while we are waiting for a WAL write inside a critical section.

We can't just defer the log message, as the IO might end up being
waited-on/completed-by another backend than the backend that issued the IO, so
we'd defer logging issues until an effectively arbitrary later time.

In general emitting a LOG inside a critical section isn't a huge issue - we
made sure that elog.c has a reserve of memory to be able to log without
crashing.

However, the current message for buffer IO issues use relpath*() (ending up in
a call to GetRelationPath()). Which in turn uses psprintf() to generate the
path. Which in turn violates the no-memory-allocations-in-critical-sections
rule, as the containing memory context will typically not have
->allowInCritSection == true.

It's not obvious to me what the best way to deal with this is.

One idea I had was to add an errrelpath() that switches to
edata->assoc_context before calling relpath(), but that would end up leaking
memory, as FreeErrorDataContents() wouldn't know about the allocation.

Obviously we could add a version of GetRelationPath() that just prints into a
caller provided buffer - but that's somewhat awkward API wise.

A third approach would be to have a dedicated memory context for this kind of
thing that's reset after logging the message - but that comes awkwardly close
to duplicating ErrorContext.

I wonder if we're lacking a bit of infrastructure here...

Greetings,

Andres Freund

Noah Misch

noah@leadboat.com

over 1 year ago

In reply to: Andres Freund (#1)

Re: GetRelationPath() vs critical sections

On Wed, Sep 04, 2024 at 11:58:33AM -0400, Andres Freund wrote:

In general emitting a LOG inside a critical section isn't a huge issue - we
made sure that elog.c has a reserve of memory to be able to log without
crashing.

However, the current message for buffer IO issues use relpath*() (ending up in
a call to GetRelationPath()). Which in turn uses psprintf() to generate the
path. Which in turn violates the no-memory-allocations-in-critical-sections
rule, as the containing memory context will typically not have
->allowInCritSection == true.

It's not obvious to me what the best way to deal with this is.

One idea I had was to add an errrelpath() that switches to
edata->assoc_context before calling relpath(), but that would end up leaking
memory, as FreeErrorDataContents() wouldn't know about the allocation.

Obviously we could add a version of GetRelationPath() that just prints into a
caller provided buffer - but that's somewhat awkward API wise.

Agreed.

A third approach would be to have a dedicated memory context for this kind of
thing that's reset after logging the message - but that comes awkwardly close
to duplicating ErrorContext.

That's how I'd try to do it. Today's ErrorContext is the context for
allocations FreeErrorDataContents() knows how to find. The new context would
be for calling into arbitrary code unknown to FreeErrorDataContents(). Most
of the time, we'd avoid reset work for the new context, since it would have no
allocations.

Ideally, errstart() would switch to the new context before returning true, and
errfinish() would switch back. That way, you could just call relpath*()
without an errrelpath() to help. This does need functions called in ereport()
argument lists to not use CurrentMemoryContext for allocations that need to
survive longer. I'd not be concerned about imposing that in a major release.
What obstacles would arise if we did that?

I wonder if we're lacking a bit of infrastructure here...

Conceptually, the ereport() argument list should be a closure that runs in a
suitable mcxt. I think we're not far from the goal.

Thomas Munro

thomas.munro@gmail.com

over 1 year ago

In reply to: Andres Freund (#1)

Re: GetRelationPath() vs critical sections

On Thu, Sep 5, 2024 at 3:58 AM Andres Freund <andres@anarazel.de> wrote:

Obviously we could add a version of GetRelationPath() that just prints into a
caller provided buffer - but that's somewhat awkward API wise.

For the record, that's exactly what I did in the patch I proposed to
fix our long standing RelationTruncate() data-eating bug:

/messages/by-id/CA+hUKG+5nfWcpnZ=Z=UpGvY1tTF=4QU_0U_07EFaKmH7Nr+NLQ@mail.gmail.com

Andy Fan

zhihuifan1213@163.com

over 1 year ago

In reply to: Thomas Munro (#3)

Re: GetRelationPath() vs critical sections

Thomas Munro <thomas.munro@gmail.com> writes:

On Thu, Sep 5, 2024 at 3:58 AM Andres Freund <andres@anarazel.de> wrote:

Obviously we could add a version of GetRelationPath() that just prints into a
caller provided buffer - but that's somewhat awkward API wise.

For the record, that's exactly what I did in the patch I proposed to
fix our long standing RelationTruncate() data-eating bug:

/messages/by-id/CA+hUKG+5nfWcpnZ=Z=UpGvY1tTF=4QU_0U_07EFaKmH7Nr+NLQ@mail.gmail.com

I want to have a dicussion on the user provided buffer APIs. I just get
the similar feedback on [1]/messages/by-id/1882669.1726701697@sss.pgh.pa.us because of this recently..

I agree that "user provided buffer" API is bad for the reasons like:
a). inconvenient since user need to provide the buffer. b) unsafe
because user may provide a incorrect buffer. But it still have some
advantages, like c). allocate the memory in a expected MemoryContext
rather than CurrentMemoryContext. d). Allocating the memory at the
different time rather than executing the API e). API can write the data
to the user descired buffer directly rather than another
copy after. My user case at [1]/messages/by-id/1882669.1726701697@sss.pgh.pa.us is because of (c) and (e), and the user
case here looks because of factor (d).

Come to the badness of "user provided buffer" API, I think we can ease
them by providing both the non-user-buffer API and user-provided-buffer
API. Since the later one is safe and convenient, so
people probably user the non-user-buffer API by default and just the
user who wants the benefits of "provided-buffer" would use that API.

Am I miss some important factors on this topic?

[1]: /messages/by-id/1882669.1726701697@sss.pgh.pa.us
/messages/by-id/1882669.1726701697@sss.pgh.pa.us

(I read the above topic [1]/messages/by-id/1882669.1726701697@sss.pgh.pa.us now, I just realize I proposed to [change] the
API rather than adding an new variant, that's not my intention and
that's my fault).

--
Best Regards
Andy Fan

Andres Freund

andres@anarazel.de

11 months ago

In reply to: Andy Fan (#4)

1 attachment(s)

Re: GetRelationPath() vs critical sections

Hi,

On 2024-10-06 11:53:59 +0800, Andy Fan wrote:

Thomas Munro <thomas.munro@gmail.com> writes:

On Thu, Sep 5, 2024 at 3:58 AM Andres Freund <andres@anarazel.de> wrote:

Obviously we could add a version of GetRelationPath() that just prints into a
caller provided buffer - but that's somewhat awkward API wise.

For the record, that's exactly what I did in the patch I proposed to
fix our long standing RelationTruncate() data-eating bug:

/messages/by-id/CA+hUKG+5nfWcpnZ=Z=UpGvY1tTF=4QU_0U_07EFaKmH7Nr+NLQ@mail.gmail.com

I want to have a dicussion on the user provided buffer APIs. I just get
the similar feedback on [1] because of this recently..

I agree that "user provided buffer" API is bad for the reasons like:
[agreed to reasons]

I didn't like *any* of the choices, including my own, this thread has
discussed so far. Needing dynamic memory allocation for this seems silly, we
know the max string length ahead of time; introducing new elog.c
infrastructure to handle the dynamic memory allocation is overkill; user
provided buffers seem too error prone; a function returning a pointer into aa
static buffer is not reentrant.

After thinking about this for an embarassingly long time, I think there's
actually a considerably better answer for a case like this: A function that
returns a fixed-length string by value:

- Compilers can fairly easily warn about on-stack values that goes out of
scope

- Because we don't need to free the memory anymore, some code that that
previously needed to explicitly free the memory doesn't need to anymore
(c.f. AbortBufferIO()).

- The max lenght isn't that long, so it's actually reasonably efficient,
likely commonly cheaper than psprintf.

In a preliminary prototype this looks ok, I created a RelPathString struct
containing a char[] of the appropriate length. That seemed less error prone
than a char[] directly, seems that could too easily decay to a pointer or
such.

An accurate length expression is somewhat awkward, but it's also not too
bad. It seems worth going for an accurate length as that makes it easier to
actually verify that length is and stays sufficient.

A hacky sketch for how this could look like is attached.

The naming certainly isn't something to actually use, but I wanted to bring
this up for discussion, before pondering that for even longer.

I converted just a few places to the new interface, there are a lot more that
will look a bit neater (or stop leaking memory, see below).

This made me realize that WaitReadBuffers() leaks a small bit of memory in the
in zero_damaged_pages path. Hardly the end of the world, but it does show how
annoying the current interface is.

Greetings,

Andres Freund

Attachments:

v1-0001-relpath-by-value.patchtext/x-diff; charset=us-asciiDownload

From d5fb6ef3324a215f7d7a5019103adac4c788df0f Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 18 Feb 2025 20:39:11 -0500
Subject: [PATCH v1] relpath-by-value

---
 src/include/common/relpath.h                 |  40 +++++
 src/common/relpath.c                         | 162 +++++++++++--------
 src/backend/storage/buffer/bufmgr.c          |  39 ++---
 src/test/regress/expected/misc_functions.out |  13 ++
 src/test/regress/regress.c                   |  14 ++
 src/test/regress/sql/misc_functions.sql      |  10 ++
 src/tools/pgindent/typedefs.list             |   1 +
 7 files changed, 183 insertions(+), 96 deletions(-)

diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h
index 5162362e7d8..9e06ecceba5 100644
--- a/src/include/common/relpath.h
+++ b/src/include/common/relpath.h
@@ -77,11 +77,51 @@ extern PGDLLIMPORT const char *const forkNames[];
 extern ForkNumber forkname_to_number(const char *forkName);
 extern int	forkname_chars(const char *str, ForkNumber *fork);
 
+/*
+ * 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);
+ */
+#define REL_PATH_STRING_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 */ \
+	+ 6 /* procNumber */ \
+	+ sizeof((char)'_') \
+	+ OIDCHARS /* relNumber */ \
+	+ sizeof((char)'_') \
+	+ FORKNAMECHARS /* forkNames[forkNumber] */ \
+		)
+
+typedef struct RelPathString
+{
+	char		path[REL_PATH_STRING_MAXLEN + 1];
+} RelPathString;
+
 /*
  * Stuff for computing filesystem pathnames for relations.
  */
 extern char *GetDatabasePath(Oid dbOid, Oid spcOid);
 
+extern RelPathString RelationPathFor(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
+									 int procNumber, ForkNumber forkNumber);
+#define relpathforperm(rlocator, forknum) \
+	RelationPathFor((rlocator).dbOid, (rlocator).spcOid, (rlocator).relNumber, \
+					INVALID_PROC_NUMBER, forknum)
+#define relpathforbackend(rlocator, backend, forknum) \
+	RelationPathFor((rlocator).dbOid, (rlocator).spcOid, (rlocator).relNumber, \
+					backend, forknum)
+
+
 extern char *GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
 							 int procNumber, ForkNumber forkNumber);
 
diff --git a/src/common/relpath.c b/src/common/relpath.c
index 186156a9e44..55c7aa1f11a 100644
--- a/src/common/relpath.c
+++ b/src/common/relpath.c
@@ -129,6 +129,96 @@ GetDatabasePath(Oid dbOid, Oid spcOid)
 	}
 }
 
+/*
+ * RelationPathFor - construct path to a relation's file
+ *
+ * The result is returned in-place as a struct, to make it suitable for use in
+ * critical sections etc.
+ *
+ * See also GetRelationPath()
+ */
+RelPathString
+RelationPathFor(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
+				int procNumber, ForkNumber forkNumber)
+{
+	RelPathString rp;
+
+	if (spcOid == GLOBALTABLESPACE_OID)
+	{
+		/* Shared system relations live in {datadir}/global */
+		Assert(dbOid == 0);
+		Assert(procNumber == INVALID_PROC_NUMBER);
+		if (forkNumber != MAIN_FORKNUM)
+			sprintf(rp.path, "global/%u_%s",
+					relNumber, forkNames[forkNumber]);
+		else
+			sprintf(rp.path, "global/%u",
+					relNumber);
+	}
+	else if (spcOid == DEFAULTTABLESPACE_OID)
+	{
+		/* The default tablespace is {datadir}/base */
+		if (procNumber == INVALID_PROC_NUMBER)
+		{
+			if (forkNumber != MAIN_FORKNUM)
+			{
+				sprintf(rp.path, "base/%u/%u_%s",
+						dbOid, relNumber,
+						forkNames[forkNumber]);
+			}
+			else
+				sprintf(rp.path, "base/%u/%u",
+						dbOid, relNumber);
+		}
+		else
+		{
+			if (forkNumber != MAIN_FORKNUM)
+				sprintf(rp.path, "base/%u/t%d_%u_%s",
+						dbOid, procNumber, relNumber,
+						forkNames[forkNumber]);
+			else
+				sprintf(rp.path, "base/%u/t%d_%u",
+						dbOid, procNumber, relNumber);
+		}
+	}
+	else
+	{
+		/* All other tablespaces are accessed via symlinks */
+		if (procNumber == INVALID_PROC_NUMBER)
+		{
+			if (forkNumber != MAIN_FORKNUM)
+				sprintf(rp.path, "%s/%u/%s/%u/%u_%s",
+						PG_TBLSPC_DIR, spcOid,
+						TABLESPACE_VERSION_DIRECTORY,
+						dbOid, relNumber,
+						forkNames[forkNumber]);
+			else
+				sprintf(rp.path, "%s/%u/%s/%u/%u",
+						PG_TBLSPC_DIR, spcOid,
+						TABLESPACE_VERSION_DIRECTORY,
+						dbOid, relNumber);
+		}
+		else
+		{
+			if (forkNumber != MAIN_FORKNUM)
+				sprintf(rp.path, "%s/%u/%s/%u/t%d_%u_%s",
+						PG_TBLSPC_DIR, spcOid,
+						TABLESPACE_VERSION_DIRECTORY,
+						dbOid, procNumber, relNumber,
+						forkNames[forkNumber]);
+			else
+				sprintf(rp.path, "%s/%u/%s/%u/t%d_%u",
+						PG_TBLSPC_DIR, spcOid,
+						TABLESPACE_VERSION_DIRECTORY,
+						dbOid, procNumber, relNumber);
+		}
+	}
+
+	Assert(strnlen(rp.path, REL_PATH_STRING_MAXLEN + 1) <= REL_PATH_STRING_MAXLEN);
+
+	return rp;
+}
+
 /*
  * GetRelationPath - construct path to a relation's file
  *
@@ -142,74 +232,6 @@ char *
 GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
 				int procNumber, ForkNumber forkNumber)
 {
-	char	   *path;
-
-	if (spcOid == GLOBALTABLESPACE_OID)
-	{
-		/* Shared system relations live in {datadir}/global */
-		Assert(dbOid == 0);
-		Assert(procNumber == INVALID_PROC_NUMBER);
-		if (forkNumber != MAIN_FORKNUM)
-			path = psprintf("global/%u_%s",
-							relNumber, forkNames[forkNumber]);
-		else
-			path = psprintf("global/%u", relNumber);
-	}
-	else if (spcOid == DEFAULTTABLESPACE_OID)
-	{
-		/* The default tablespace is {datadir}/base */
-		if (procNumber == INVALID_PROC_NUMBER)
-		{
-			if (forkNumber != MAIN_FORKNUM)
-				path = psprintf("base/%u/%u_%s",
-								dbOid, relNumber,
-								forkNames[forkNumber]);
-			else
-				path = psprintf("base/%u/%u",
-								dbOid, relNumber);
-		}
-		else
-		{
-			if (forkNumber != MAIN_FORKNUM)
-				path = psprintf("base/%u/t%d_%u_%s",
-								dbOid, procNumber, relNumber,
-								forkNames[forkNumber]);
-			else
-				path = psprintf("base/%u/t%d_%u",
-								dbOid, procNumber, relNumber);
-		}
-	}
-	else
-	{
-		/* All other tablespaces are accessed via symlinks */
-		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]);
-			else
-				path = psprintf("%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]);
-			else
-				path = psprintf("%s/%u/%s/%u/t%d_%u",
-								PG_TBLSPC_DIR, spcOid,
-								TABLESPACE_VERSION_DIRECTORY,
-								dbOid, procNumber, relNumber);
-		}
-	}
-	return path;
+	return pstrdup(RelationPathFor(dbOid, spcOid, relNumber,
+								   procNumber, forkNumber).path);
 }
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 75cfc2b6fe9..5275c06fc2f 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3663,7 +3663,6 @@ DebugPrintBufferRefcount(Buffer buffer)
 {
 	BufferDesc *buf;
 	int32		loccount;
-	char	   *path;
 	char	   *result;
 	ProcNumber	backend;
 	uint32		buf_state;
@@ -3683,15 +3682,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,
+					  relpathforbackend(BufTagGetRelFileLocator(&buf->tag), backend,
+										BufTagGetForkNum(&buf->tag)).path,
 					  buf->tag.blockNum, buf_state & BUF_FLAG_MASK,
 					  BUF_STATE_GET_REFCOUNT(buf_state), loccount);
-	pfree(path);
 	return result;
 }
 
@@ -5611,16 +5609,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,
+							relpathforperm(BufTagGetRelFileLocator(&buf_hdr->tag),
+										   BufTagGetForkNum(&buf_hdr->tag)).path),
 					 errdetail("Multiple failures --- write error might be permanent.")));
-			pfree(path);
 		}
 	}
 
@@ -5637,14 +5632,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,
+				   relpathforperm(BufTagGetRelFileLocator(&bufHdr->tag),
+								  BufTagGetForkNum(&bufHdr->tag)).path);
 }
 
 /*
@@ -5656,15 +5647,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,
+				   relpathforbackend(BufTagGetRelFileLocator(&bufHdr->tag),
+									 MyProcNumber,
+									 BufTagGetForkNum(&bufHdr->tag)).path);
 }
 
 /*
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index 106dedb519a..64e4d757ebf 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -903,3 +903,16 @@ SELECT gist_stratnum_common(3);
                    18
 (1 row)
 
+-- relpath tests
+CREATE FUNCTION test_relpath()
+    RETURNS text
+    AS :'regresslib'
+    LANGUAGE C;
+-- FIXME: This would include version number, which we don't want in the
+-- expected file.
+SELECT test_relpath();
+                              test_relpath                               
+-------------------------------------------------------------------------
+ pg_tblspc/4294967295/PG_18_202502112/4294967295/t262142_4294967295_init
+(1 row)
+
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 667d9835148..b8d2a1725fe 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -1208,3 +1208,17 @@ binary_coercible(PG_FUNCTION_ARGS)
 
 	PG_RETURN_BOOL(IsBinaryCoercible(srctype, targettype));
 }
+
+#include "postmaster/postmaster.h"
+
+PG_FUNCTION_INFO_V1(test_relpath);
+Datum
+test_relpath(PG_FUNCTION_ARGS)
+{
+	RelPathString rpath;
+
+	rpath = RelationPathFor(OID_MAX, OID_MAX, OID_MAX, MAX_BACKENDS - 1,
+							INIT_FORKNUM);
+
+	PG_RETURN_DATUM(CStringGetTextDatum(rpath.path));
+}
diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql
index 753a0f41c03..e7a2e4fb7c2 100644
--- a/src/test/regress/sql/misc_functions.sql
+++ b/src/test/regress/sql/misc_functions.sql
@@ -403,3 +403,13 @@ 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 text
+    AS :'regresslib'
+    LANGUAGE C;
+-- FIXME: This would include version number, which we don't want in the
+-- expected file.
+SELECT test_relpath();
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 80aa50d55a4..ce75ce75e8f 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2402,6 +2402,7 @@ RelMapFile
 RelMapping
 RelOptInfo
 RelOptKind
+RelPathString
 RelToCheck
 RelToCluster
 RelabelType
-- 
2.48.1.76.g4e746b1a31.dirty

Thomas Munro

thomas.munro@gmail.com

11 months ago

In reply to: Andres Freund (#5)

Re: GetRelationPath() vs critical sections

On Wed, Feb 19, 2025 at 3:35 PM Andres Freund <andres@anarazel.de> wrote:

After thinking about this for an embarassingly long time, I think there's
actually a considerably better answer for a case like this: A function that
returns a fixed-length string by value:

- Compilers can fairly easily warn about on-stack values that goes out of
scope

- Because we don't need to free the memory anymore, some code that that
previously needed to explicitly free the memory doesn't need to anymore
(c.f. AbortBufferIO()).

- The max lenght isn't that long, so it's actually reasonably efficient,
likely commonly cheaper than psprintf.

I like it!

This made me realize that WaitReadBuffers() leaks a small bit of memory in the
in zero_damaged_pages path. Hardly the end of the world, but it does show how
annoying the current interface is.

Right. It uses relpath() for an error message, but unlike similar
examples this one is only a WARNING so it might run an unbounded
number of times before the context cleans the memory up. Not new
code, just moved around in v17.

Andres Freund

andres@anarazel.de

11 months ago

In reply to: Thomas Munro (#6)

Re: GetRelationPath() vs critical sections

Hi,

On 2025-02-20 14:00:10 +1300, Thomas Munro wrote:

On Wed, Feb 19, 2025 at 3:35 PM Andres Freund <andres@anarazel.de> wrote:

After thinking about this for an embarassingly long time, I think there's
actually a considerably better answer for a case like this: A function that
returns a fixed-length string by value:

- Compilers can fairly easily warn about on-stack values that goes out of
scope

- Because we don't need to free the memory anymore, some code that that
previously needed to explicitly free the memory doesn't need to anymore
(c.f. AbortBufferIO()).

- The max lenght isn't that long, so it's actually reasonably efficient,
likely commonly cheaper than psprintf.

I like it!

Does anybody have opinions about whether we should keep a backward compatible
interface in place or not?

Via https://codesearch.debian.net/ I tried to look for
references.

Unfortunately I had to exclude "relpath" as there are just too many
independent hits, due to the python function of the same name. For
relpathperm(), relpathbackend(), GetRelationPath() there looks to be just
fincore. There also are two copies of our code, but those we don't need to
care about (libpg-query and ruby-pg-query), since they're just going to copy
the new code when updating.

I also looked for matches including relpath that had "fork" elsewhere in the
file, it's hard to see a potential use of relpath() not using fork in the
arguments or such.

Which makes me think it's not worth having a backward compatible interface?

Greetings,

Andres Freund

Noah Misch

noah@leadboat.com

11 months ago

In reply to: Andres Freund (#7)

Re: GetRelationPath() vs critical sections

On Thu, Feb 20, 2025 at 12:40:57PM -0500, Andres Freund wrote:

On 2025-02-20 14:00:10 +1300, Thomas Munro wrote:

On Wed, Feb 19, 2025 at 3:35 PM Andres Freund <andres@anarazel.de> wrote:

After thinking about this for an embarassingly long time, I think there's
actually a considerably better answer for a case like this: A function that
returns a fixed-length string by value:

- Compilers can fairly easily warn about on-stack values that goes out of
scope

- Because we don't need to free the memory anymore, some code that that
previously needed to explicitly free the memory doesn't need to anymore
(c.f. AbortBufferIO()).

- The max lenght isn't that long, so it's actually reasonably efficient,
likely commonly cheaper than psprintf.

I like it!

Works for me.

Unfortunately I had to exclude "relpath" as there are just too many
independent hits, due to the python function of the same name. For
relpathperm(), relpathbackend(), GetRelationPath() there looks to be just
fincore.

PGXN has few hits, and some of these are false positives or otherwise
irrelevant:

$ grep -re '[^.]$relpath[a-z]*\|GetRelationPath$(' | sed 's/-[^:]*/:/'|sort -u
db2_fdw::extern char *GetRelationPath(Oid dbNode, Oid spcNode, Oid relNode,
jsoncdc:: pub fn GetRelationPath(dbNode: Oid, spcNode: Oid, relNode: Oid,
openbarter:: path = relpath(frame.f_code.co_filename, refdir) # relative to refdir
pg_bulkload::#define relpath(rnode, forknum) relpath((rnode))
pg_bulkload:: fname = relpath(bknode, MAIN_FORKNUM);
pg_bulkload:: fname = relpath(rnode, MAIN_FORKNUM);
pg_repack::#define relpath(rnode, forknum) relpath((rnode))
plv8:: def _to_relpath(self, abspath, _):
plv8:: def _to_relpath(self, abspath, test_root):
plv8:: yield self._to_relpath(abspath, test_root)
tblsize_nolock:: relationpath = relpath(*rfn);

Which makes me think it's not worth having a backward compatible interface?

Agreed. Even if 100% of those matches had to change, that's below standard
level of breakage for a major release.

Tom Lane

tgl@sss.pgh.pa.us

11 months ago

In reply to: Andres Freund (#7)

Re: GetRelationPath() vs critical sections

Andres Freund <andres@anarazel.de> writes:

Does anybody have opinions about whether we should keep a backward compatible
interface in place or not?

I vote for "not" --- doesn't seem like there'll be much external
code affected, and we make comparably-sized API breaks all the time.

As a matter of style, I wonder if it'd be better to have these
functions write into a caller-supplied variable. 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 realize that returning structs has been in C for decades,
but that doesn't mean I want some of our APIs doing it one way and
some the other.

regards, tom lane

#10

Andres Freund

andres@anarazel.de

11 months ago

In reply to: Tom Lane (#9)

2 attachment(s)

Re: GetRelationPath() vs critical sections

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

Attachments:

v2-0001-Change-relpath-etc-to-return-path-by-value.patchtext/x-diff; charset=us-asciiDownload

From e25fe65ef66e536ef6074a3f273c5cecd27f65b7 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@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

v2-0002-Change-_mdfd_segpath-to-return-paths-by-value.patchtext/x-diff; charset=us-asciiDownload

From ee29373d7be39bac811c0dc2edb62915e916b151 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@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

#11

Thomas Munro

thomas.munro@gmail.com

11 months ago

In reply to: Andres Freund (#10)

1 attachment(s)

Re: GetRelationPath() vs critical sections

On Fri, Feb 21, 2025 at 9:28 AM Andres Freund <andres@anarazel.de> wrote:

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

Do we even check the binary digits? BTW I see a place in lwlock.c
that still talks about 2^23-1, looks like it is out of date. Hmmm, I
wonder if it would be better to start by declaring how many bits we
want to use, given that is our real constraint. And then:

#define PROCNUMBER_BITS 18
#define MAX_BACKENDS ((1 << PROCNUMBER_BITS) - 1)
#define PROCNUMBER_CHARS DECIMAL_DIGITS_FOR_BITS(PROCNUMBER_BITS)

... with a little helper ported to preprocessor hell from Hacker's
Delight magic[1]https://lemire.me/blog/2021/06/03/computing-the-number-of-digits-of-an-integer-even-faster/ for that last bit. See attached. But if that's a
bit too nuts...

b) Use a static assert to check that it fits?

Right, easy stuff like sizeof(CppString2(MAX_BACKENDS)) - 1 can only
work if the token is a decimal number. I suppose you could just use
constants:

#define MAX_BACKENDS 0x3ffff
#define PROCNUMBER_BITS 18
#define PROCNUMBER_CHARS 6

... and then use the previous magic to statically assert their
relationship inside one translation unit, to keep it out of a widely
included header.

[1]: https://lemire.me/blog/2021/06/03/computing-the-number-of-digits-of-an-integer-even-faster/

#12

Tom Lane

tgl@sss.pgh.pa.us

11 months ago

In reply to: Thomas Munro (#11)

Re: GetRelationPath() vs critical sections

Thomas Munro <thomas.munro@gmail.com> writes:

On Fri, Feb 21, 2025 at 9:28 AM Andres Freund <andres@anarazel.de> wrote:

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

This all seems quite over-the-top to me. Just allocate 10 characters,
which is certainly enough for the output of a %u format spec, and
call it good.

(Yeah, I know that's not as much fun, but ...)

regards, tom lane

#13

Thomas Munro

thomas.munro@gmail.com

11 months ago

In reply to: Thomas Munro (#11)

Re: GetRelationPath() vs critical sections

On Fri, Feb 21, 2025 at 6:20 PM Thomas Munro <thomas.munro@gmail.com> wrote:

#define PROCNUMBER_BITS 18
#define MAX_BACKENDS ((1 << PROCNUMBER_BITS) - 1)
#define PROCNUMBER_CHARS DECIMAL_DIGITS_FOR_BITS(PROCNUMBER_BITS)

... with a little helper ported to preprocessor hell from Hacker's
Delight magic[1] for that last bit. See attached. But if that's a
bit too nuts...

Erm, wait of course you just need:

#define DECIMAL_DIGITS(n) \
((n) < 10 ? 1 : \
(n) < 100 ? 2 : \
(n) < 1000 ? 3 : \
(n) < 10000 ? 4 : \
(n) < 100000 ? 5 : \
(n) < 1000000 ? 6 : \
(n) < 10000000 ? 7 : \
(n) < 100000000 ? 8 : \
(n) < 1000000000 ? 9 : 10)

#define PROCNUMBER_BITS 18
#define MAX_BACKENDS ((1 << PROCNUMBER_BITS) - 1)
#define PROCNUMBER_CHARS DECIMAL_DIGITS(MAX_BACKENDS)

#14

Andres Freund

andres@anarazel.de

11 months ago

In reply to: Thomas Munro (#11)

Re: GetRelationPath() vs critical sections

Hi,

On 2025-02-21 18:20:39 +1300, Thomas Munro wrote:

On Fri, Feb 21, 2025 at 9:28 AM Andres Freund <andres@anarazel.de> wrote:

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

Do we even check the binary digits? BTW I see a place in lwlock.c
that still talks about 2^23-1, looks like it is out of date.

Yea. I actually posted a patch addressing that recently-ish:
/messages/by-id/7bye3hvkdk7ybt4ofigimr4ahshsj657aqatfc5trd5pbp5qd4@dir2wsx2lgo6

Patch 0001 in that series would be nice to have here, because it moves
MAX_BACKENDS out of postmaster.h. I kind had hoped somebody would comment on
whether pg_config_manual.h is a good place for the define.

I guess we could also move it to storage/procnumber.h.

Hmmm, I wonder if it would be better to start by declaring how many bits we
want to use, given that is our real constraint. And then:

#define PROCNUMBER_BITS 18
#define MAX_BACKENDS ((1 << PROCNUMBER_BITS) - 1)
#define PROCNUMBER_CHARS DECIMAL_DIGITS_FOR_BITS(PROCNUMBER_BITS)

... with a little helper ported to preprocessor hell from Hacker's
Delight magic[1] for that last bit. See attached. But if that's a
bit too nuts...

This seems a bit too complicated to be worth it. I am inclined to think the
approach taken in the patch of just having a regression test verifying that
the numbers match is good enough.

The one arguments I can see for something like DECIMAL_DIGITS_TABLE is that we
could define OIDCHARS the same way. With a quick grep I couldn't immediately
find other candidates though.

b) Use a static assert to check that it fits?

Right, easy stuff like sizeof(CppString2(MAX_BACKENDS)) - 1 can only
work if the token is a decimal number. I suppose you could just use
constants:

#define MAX_BACKENDS 0x3ffff
#define PROCNUMBER_BITS 18
#define PROCNUMBER_CHARS 6

... and then use the previous magic to statically assert their
relationship inside one translation unit, to keep it out of a widely
included header.

I think we could also just define MAX_BACKENDS as a decimal number. I don't
find 0x3ffff that meaningfully better than 262143

Greetings,

Andres Freund

#15

Thomas Munro

thomas.munro@gmail.com

11 months ago

In reply to: Andres Freund (#14)

Re: GetRelationPath() vs critical sections

On Sat, Feb 22, 2025 at 5:15 AM Andres Freund <andres@anarazel.de> wrote:

On 2025-02-21 18:20:39 +1300, Thomas Munro wrote:

On Fri, Feb 21, 2025 at 9:28 AM Andres Freund <andres@anarazel.de> wrote:
Do we even check the binary digits? BTW I see a place in lwlock.c
that still talks about 2^23-1, looks like it is out of date.

Yea. I actually posted a patch addressing that recently-ish:
/messages/by-id/7bye3hvkdk7ybt4ofigimr4ahshsj657aqatfc5trd5pbp5qd4@dir2wsx2lgo6

Patch 0001 in that series would be nice to have here, because it moves
MAX_BACKENDS out of postmaster.h. I kind had hoped somebody would comment on
whether pg_config_manual.h is a good place for the define.

I guess we could also move it to storage/procnumber.h.

procnumber.h seems like the right place, at least without a separate
discussion of the ramifications of making it configurable, no? (I
thought there were ideas about squeezing it down to 16 bits so you
could jam two of 'em into an atomic uint32_t for list headers or
something like that, off-topic here except to say that it seems to
conflict with the idea of making it user-increasable?)

Hmmm, I wonder if it would be better to start by declaring how many bits we
want to use, given that is our real constraint. And then:

#define PROCNUMBER_BITS 18
#define MAX_BACKENDS ((1 << PROCNUMBER_BITS) - 1)
#define PROCNUMBER_CHARS DECIMAL_DIGITS_FOR_BITS(PROCNUMBER_BITS)

... with a little helper ported to preprocessor hell from Hacker's
Delight magic[1] for that last bit. See attached. But if that's a
bit too nuts...

This seems a bit too complicated to be worth it. I am inclined to think the
approach taken in the patch of just having a regression test verifying that
the numbers match is good enough.

The one arguments I can see for something like DECIMAL_DIGITS_TABLE is that we
could define OIDCHARS the same way. With a quick grep I couldn't immediately
find other candidates though.

If anyone ever does consider using something like that, whether to
derive values or just to make assertions that they agree inside a
translation unit somewhere, FTR here's why I posted the simple version
after being temporarily nerdsniped by Hacker's Delight: the
bitswizzling algorithm was translated from branch-free code, which is
fun but irrelevant here and that property didn't even survive the
translation. So you might as well just use the naive definition of
DECIMAL_DIGITS(n) I posted immediately after.

#16

Thomas Munro

thomas.munro@gmail.com

11 months ago

In reply to: Thomas Munro (#15)

Re: GetRelationPath() vs critical sections

On Sat, Feb 22, 2025 at 1:30 PM Thomas Munro <thomas.munro@gmail.com> wrote:

procnumber.h seems like the right place, at least without a separate
discussion of the ramifications of making it configurable, no? (I
thought there were ideas about squeezing it down to 16 bits so you
could jam two of 'em into an atomic uint32_t for list headers or
something like that, off-topic here except to say that it seems to
conflict with the idea of making it user-increasable?)

(Please ignore this comment, I'll comment on the other thread instead.
Sorry I hadn't seen your patches over there yet: when I started
talking about the definition and assertions around MAX_BACKENDS in
here, it had jumped out at me independently while trying to answer
your question about compile-time log10 stuff, because I also noticed
that we sucked even at codifying the log2 constraints.)

#17

Andres Freund

andres@anarazel.de

11 months ago

In reply to: Andres Freund (#10)

2 attachment(s)

Re: GetRelationPath() vs critical sections

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

Attachments:

v3-0001-Change-relpath-et-al-to-return-path-by-value.patchtext/x-diff; charset=us-asciiDownload

From a94e33869aac2fb8e961eb25e7dd75261339f241 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@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

v3-0002-Change-_mdfd_segpath-to-return-paths-by-value.patchtext/x-diff; charset=us-asciiDownload

From bce2daeac10134c588a787585461f0ae16519491 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@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

#18

Andres Freund

andres@anarazel.de

11 months ago

In reply to: Andres Freund (#17)

Re: GetRelationPath() vs critical sections

Hi,

On 2025-02-24 10:50:21 -0500, Andres Freund wrote:

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.

And pushed.

Greetings,

Andres Freund