Hi,
On 2024-10-06 11:53:59 +0800, Andy Fan wrote:
> Thomas Munro <thomas.mu...@gmail.com> writes:
>
> > On Thu, Sep 5, 2024 at 3:58 AM Andres Freund <and...@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:
> >
> > https://www.postgresql.org/message-id/flat/CA%2BhUKG%2B5nfWcpnZ%3DZ%3DUpGvY1tTF%3D4QU_0U_07EFaKmH7Nr%2BNLQ%40mail.gmail.com#aa061db119ee7a4b5390af56e24f475d
>
> 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
>From d5fb6ef3324a215f7d7a5019103adac4c788df0f Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@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
