On 2012-12-11 16:43:12 +0100, Andres Freund wrote:
> On 2012-12-11 15:44:39 +0100, Andres Freund wrote:
> > On 2012-12-11 15:55:35 +0200, Heikki Linnakangas wrote:
> > > * It's pretty ugly that to use the rm_desc functions, you have to provide
> > > dummy implementations of a bunch of backend functions, including pfree()
> > > and
> > > timestamptz_to_str(). Should find a better way to do that.
> >
> > I think most of the cases requiring those ugly hacks can be fixed to
> > just use a caller-provided buffer, there's not that much left.
> >
> > timestamptz_to_str() is probably the most complex case. I just noticed
> > there's already a second implementation in
> > ecpg/pgtypeslib/dt_common.c. Yuck. It seems to already have diverged in
> > a number of cases :(
>
> The attached (and pushed) patches change relpathbackend to use a static buffer
> instead. That gets rid of the pfree() requirement and looks ok otherwise as
> well.
... really attached.
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 1ec1700605bc8e69e2df1ff08d1359d90c9a1ca3 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Tue, 11 Dec 2012 16:36:06 +0100
Subject: [PATCH 1/2] Make relpathbackend return a statically result instead
of palloc()'ing it
relpathbackend() (via some of its wrappers) is used in *_desc routines which we
want to be useable without a backend environment arround.
Change signature to return a 'const char *' to make misuse easier to
detect. That necessicates also changing the 'FileName' typedef to 'const char
*' which seems to be a good idea anyway.
---
src/backend/access/rmgrdesc/smgrdesc.c | 6 ++--
src/backend/access/rmgrdesc/xactdesc.c | 6 ++--
src/backend/access/transam/xlogutils.c | 9 ++----
src/backend/catalog/catalog.c | 48 ++++++++++----------------------
src/backend/storage/buffer/bufmgr.c | 12 +++-----
src/backend/storage/smgr/md.c | 23 +++++----------
src/backend/utils/adt/dbsize.c | 4 +--
src/include/catalog/catalog.h | 2 +-
src/include/storage/fd.h | 2 +-
9 files changed, 37 insertions(+), 75 deletions(-)
diff --git a/src/backend/access/rmgrdesc/smgrdesc.c b/src/backend/access/rmgrdesc/smgrdesc.c
index 40b9708..da53a48 100644
--- a/src/backend/access/rmgrdesc/smgrdesc.c
+++ b/src/backend/access/rmgrdesc/smgrdesc.c
@@ -26,19 +26,17 @@ smgr_desc(StringInfo buf, uint8 xl_info, char *rec)
if (info == XLOG_SMGR_CREATE)
{
xl_smgr_create *xlrec = (xl_smgr_create *) rec;
- char *path = relpathperm(xlrec->rnode, xlrec->forkNum);
+ const char *path = relpathperm(xlrec->rnode, xlrec->forkNum);
appendStringInfo(buf, "file create: %s", path);
- pfree(path);
}
else if (info == XLOG_SMGR_TRUNCATE)
{
xl_smgr_truncate *xlrec = (xl_smgr_truncate *) rec;
- char *path = relpathperm(xlrec->rnode, MAIN_FORKNUM);
+ const char *path = relpathperm(xlrec->rnode, MAIN_FORKNUM);
appendStringInfo(buf, "file truncate: %s to %u blocks", path,
xlrec->blkno);
- pfree(path);
}
else
appendStringInfo(buf, "UNKNOWN");
diff --git a/src/backend/access/rmgrdesc/xactdesc.c b/src/backend/access/rmgrdesc/xactdesc.c
index 60deddc..7cad3e5 100644
--- a/src/backend/access/rmgrdesc/xactdesc.c
+++ b/src/backend/access/rmgrdesc/xactdesc.c
@@ -35,10 +35,9 @@ xact_desc_commit(StringInfo buf, xl_xact_commit *xlrec)
appendStringInfo(buf, "; rels:");
for (i = 0; i < xlrec->nrels; i++)
{
- char *path = relpathperm(xlrec->xnodes[i], MAIN_FORKNUM);
+ const char *path = relpathperm(xlrec->xnodes[i], MAIN_FORKNUM);
appendStringInfo(buf, " %s", path);
- pfree(path);
}
}
if (xlrec->nsubxacts > 0)
@@ -105,10 +104,9 @@ xact_desc_abort(StringInfo buf, xl_xact_abort *xlrec)
appendStringInfo(buf, "; rels:");
for (i = 0; i < xlrec->nrels; i++)
{
- char *path = relpathperm(xlrec->xnodes[i], MAIN_FORKNUM);
+ const char *path = relpathperm(xlrec->xnodes[i], MAIN_FORKNUM);
appendStringInfo(buf, " %s", path);
- pfree(path);
}
}
if (xlrec->nsubxacts > 0)
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 5676120..43c7254 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -57,7 +57,7 @@ static void
report_invalid_page(int elevel, RelFileNode node, ForkNumber forkno,
BlockNumber blkno, bool present)
{
- char *path = relpathperm(node, forkno);
+ const char *path = relpathperm(node, forkno);
if (present)
elog(elevel, "page %u of relation %s is uninitialized",
@@ -65,7 +65,6 @@ report_invalid_page(int elevel, RelFileNode node, ForkNumber forkno,
else
elog(elevel, "page %u of relation %s does not exist",
blkno, path);
- pfree(path);
}
/* Log a reference to an invalid page */
@@ -153,11 +152,10 @@ forget_invalid_pages(RelFileNode node, ForkNumber forkno, BlockNumber minblkno)
{
if (log_min_messages <= DEBUG2 || client_min_messages <= DEBUG2)
{
- char *path = relpathperm(hentry->key.node, forkno);
+ const char *path = relpathperm(hentry->key.node, forkno);
elog(DEBUG2, "page %u of relation %s has been dropped",
hentry->key.blkno, path);
- pfree(path);
}
if (hash_search(invalid_page_tab,
@@ -186,11 +184,10 @@ forget_invalid_pages_db(Oid dbid)
{
if (log_min_messages <= DEBUG2 || client_min_messages <= DEBUG2)
{
- char *path = relpathperm(hentry->key.node, hentry->key.forkno);
+ const char *path = relpathperm(hentry->key.node, hentry->key.forkno);
elog(DEBUG2, "page %u of relation %s has been dropped",
hentry->key.blkno, path);
- pfree(path);
}
if (hash_search(invalid_page_tab,
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 79b71b3..21c512e 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -112,54 +112,45 @@ forkname_chars(const char *str, ForkNumber *fork)
/*
* relpathbackend - construct path to a relation's file
*
- * Result is a palloc'd string.
+ * Result is a pointer to a statically allocated string.
*/
-char *
+const char *
relpathbackend(RelFileNode rnode, BackendId backend, ForkNumber forknum)
{
- int pathlen;
- char *path;
+ static char path[MAXPGPATH];
if (rnode.spcNode == GLOBALTABLESPACE_OID)
{
/* Shared system relations live in {datadir}/global */
Assert(rnode.dbNode == 0);
Assert(backend == InvalidBackendId);
- pathlen = 7 + OIDCHARS + 1 + FORKNAMECHARS + 1;
- path = (char *) palloc(pathlen);
if (forknum != MAIN_FORKNUM)
- snprintf(path, pathlen, "global/%u_%s",
+ snprintf(path, MAXPGPATH, "global/%u_%s",
rnode.relNode, forkNames[forknum]);
else
- snprintf(path, pathlen, "global/%u", rnode.relNode);
+ snprintf(path, MAXPGPATH, "global/%u", rnode.relNode);
}
else if (rnode.spcNode == DEFAULTTABLESPACE_OID)
{
/* The default tablespace is {datadir}/base */
if (backend == InvalidBackendId)
{
- pathlen = 5 + OIDCHARS + 1 + OIDCHARS + 1 + FORKNAMECHARS + 1;
- path = (char *) palloc(pathlen);
if (forknum != MAIN_FORKNUM)
- snprintf(path, pathlen, "base/%u/%u_%s",
+ snprintf(path, MAXPGPATH, "base/%u/%u_%s",
rnode.dbNode, rnode.relNode,
forkNames[forknum]);
else
- snprintf(path, pathlen, "base/%u/%u",
+ snprintf(path, MAXPGPATH, "base/%u/%u",
rnode.dbNode, rnode.relNode);
}
else
{
- /* OIDCHARS will suffice for an integer, too */
- pathlen = 5 + OIDCHARS + 2 + OIDCHARS + 1 + OIDCHARS + 1
- + FORKNAMECHARS + 1;
- path = (char *) palloc(pathlen);
if (forknum != MAIN_FORKNUM)
- snprintf(path, pathlen, "base/%u/t%d_%u_%s",
+ snprintf(path, MAXPGPATH, "base/%u/t%d_%u_%s",
rnode.dbNode, backend, rnode.relNode,
forkNames[forknum]);
else
- snprintf(path, pathlen, "base/%u/t%d_%u",
+ snprintf(path, MAXPGPATH, "base/%u/t%d_%u",
rnode.dbNode, backend, rnode.relNode);
}
}
@@ -168,38 +159,31 @@ relpathbackend(RelFileNode rnode, BackendId backend, ForkNumber forknum)
/* All other tablespaces are accessed via symlinks */
if (backend == InvalidBackendId)
{
- pathlen = 9 + 1 + OIDCHARS + 1
- + strlen(TABLESPACE_VERSION_DIRECTORY) + 1 + OIDCHARS + 1
- + OIDCHARS + 1 + FORKNAMECHARS + 1;
- path = (char *) palloc(pathlen);
if (forknum != MAIN_FORKNUM)
- snprintf(path, pathlen, "pg_tblspc/%u/%s/%u/%u_%s",
+ snprintf(path, MAXPGPATH, "pg_tblspc/%u/%s/%u/%u_%s",
rnode.spcNode, TABLESPACE_VERSION_DIRECTORY,
rnode.dbNode, rnode.relNode,
forkNames[forknum]);
else
- snprintf(path, pathlen, "pg_tblspc/%u/%s/%u/%u",
+ snprintf(path, MAXPGPATH, "pg_tblspc/%u/%s/%u/%u",
rnode.spcNode, TABLESPACE_VERSION_DIRECTORY,
rnode.dbNode, rnode.relNode);
}
else
{
/* OIDCHARS will suffice for an integer, too */
- pathlen = 9 + 1 + OIDCHARS + 1
- + strlen(TABLESPACE_VERSION_DIRECTORY) + 1 + OIDCHARS + 2
- + OIDCHARS + 1 + OIDCHARS + 1 + FORKNAMECHARS + 1;
- path = (char *) palloc(pathlen);
if (forknum != MAIN_FORKNUM)
- snprintf(path, pathlen, "pg_tblspc/%u/%s/%u/t%d_%u_%s",
+ snprintf(path, MAXPGPATH, "pg_tblspc/%u/%s/%u/t%d_%u_%s",
rnode.spcNode, TABLESPACE_VERSION_DIRECTORY,
rnode.dbNode, backend, rnode.relNode,
forkNames[forknum]);
else
- snprintf(path, pathlen, "pg_tblspc/%u/%s/%u/t%d_%u",
+ snprintf(path, MAXPGPATH, "pg_tblspc/%u/%s/%u/t%d_%u",
rnode.spcNode, TABLESPACE_VERSION_DIRECTORY,
rnode.dbNode, backend, rnode.relNode);
}
}
+
return path;
}
@@ -534,7 +518,7 @@ Oid
GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
{
RelFileNodeBackend rnode;
- char *rpath;
+ const char *rpath;
int fd;
bool collides;
BackendId backend;
@@ -599,8 +583,6 @@ GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
*/
collides = false;
}
-
- pfree(rpath);
} while (collides);
return rnode.node.relNode;
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index dddb6c0..7be767b 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1757,7 +1757,7 @@ PrintBufferLeakWarning(Buffer buffer)
{
volatile BufferDesc *buf;
int32 loccount;
- char *path;
+ const char *path;
BackendId backend;
Assert(BufferIsValid(buffer));
@@ -1782,7 +1782,6 @@ PrintBufferLeakWarning(Buffer buffer)
buffer, path,
buf->tag.blockNum, buf->flags,
buf->refcount, loccount);
- pfree(path);
}
/*
@@ -2901,7 +2900,7 @@ AbortBufferIO(void)
if (sv_flags & BM_IO_ERROR)
{
/* Buffer is pinned, so we can read tag without spinlock */
- char *path;
+ const char *path;
path = relpathperm(buf->tag.rnode, buf->tag.forkNum);
ereport(WARNING,
@@ -2909,7 +2908,6 @@ AbortBufferIO(void)
errmsg("could not write block %u of %s",
buf->tag.blockNum, path),
errdetail("Multiple failures --- write error might be permanent.")));
- pfree(path);
}
}
TerminateBufferIO(buf, false, BM_IO_ERROR);
@@ -2927,11 +2925,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(bufHdr->tag.rnode, bufHdr->tag.forkNum);
+ const char *path = relpathperm(bufHdr->tag.rnode, bufHdr->tag.forkNum);
errcontext("writing block %u of relation %s",
bufHdr->tag.blockNum, path);
- pfree(path);
}
}
@@ -2945,11 +2942,10 @@ local_buffer_write_error_callback(void *arg)
if (bufHdr != NULL)
{
- char *path = relpathbackend(bufHdr->tag.rnode, MyBackendId,
+ const char *path = relpathbackend(bufHdr->tag.rnode, MyBackendId,
bufHdr->tag.forkNum);
errcontext("writing block %u of relation %s",
bufHdr->tag.blockNum, path);
- pfree(path);
}
}
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 384acae..90267fd 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -264,7 +264,7 @@ mdexists(SMgrRelation reln, ForkNumber forkNum)
void
mdcreate(SMgrRelation reln, ForkNumber forkNum, bool isRedo)
{
- char *path;
+ const char *path;
File fd;
if (isRedo && reln->md_fd[forkNum] != NULL)
@@ -298,8 +298,6 @@ mdcreate(SMgrRelation reln, ForkNumber forkNum, bool isRedo)
}
}
- pfree(path);
-
reln->md_fd[forkNum] = _fdvec_alloc();
reln->md_fd[forkNum]->mdfd_vfd = fd;
@@ -380,7 +378,7 @@ mdunlink(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
static void
mdunlinkfork(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
{
- char *path;
+ const char *path;
int ret;
path = relpath(rnode, forkNum);
@@ -449,8 +447,6 @@ mdunlinkfork(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
}
pfree(segpath);
}
-
- pfree(path);
}
/*
@@ -545,7 +541,7 @@ static MdfdVec *
mdopen(SMgrRelation reln, ForkNumber forknum, ExtensionBehavior behavior)
{
MdfdVec *mdfd;
- char *path;
+ const char *path;
File fd;
/* No work if already open */
@@ -571,7 +567,6 @@ mdopen(SMgrRelation reln, ForkNumber forknum, ExtensionBehavior behavior)
if (behavior == EXTENSION_RETURN_NULL &&
FILE_POSSIBLY_DELETED(errno))
{
- pfree(path);
return NULL;
}
ereport(ERROR,
@@ -580,8 +575,6 @@ mdopen(SMgrRelation reln, ForkNumber forknum, ExtensionBehavior behavior)
}
}
- pfree(path);
-
reln->md_fd[forknum] = mdfd = _fdvec_alloc();
mdfd->mdfd_vfd = fd;
@@ -1279,7 +1272,7 @@ mdpostckpt(void)
while (pendingUnlinks != NIL)
{
PendingUnlinkEntry *entry = (PendingUnlinkEntry *) linitial(pendingUnlinks);
- char *path;
+ const char *path;
/*
* New entries are appended to the end, so if the entry is new we've
@@ -1309,7 +1302,6 @@ mdpostckpt(void)
(errcode_for_file_access(),
errmsg("could not remove file \"%s\": %m", path)));
}
- pfree(path);
/* And remove the list entry */
pendingUnlinks = list_delete_first(pendingUnlinks);
@@ -1634,8 +1626,8 @@ _fdvec_alloc(void)
static char *
_mdfd_segpath(SMgrRelation reln, ForkNumber forknum, BlockNumber segno)
{
- char *path,
- *fullpath;
+ const char *path;
+ char *fullpath;
path = relpath(reln->smgr_rnode, forknum);
@@ -1644,10 +1636,9 @@ _mdfd_segpath(SMgrRelation reln, ForkNumber forknum, BlockNumber segno)
/* be sure we have enough space for the '.segno' */
fullpath = (char *) palloc(strlen(path) + 12);
sprintf(fullpath, "%s.%u", path, segno);
- pfree(path);
}
else
- fullpath = path;
+ fullpath = pstrdup(path);
return fullpath;
}
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index cd23334..c3ee640 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -265,7 +265,7 @@ static int64
calculate_relation_size(RelFileNode *rfn, BackendId backend, ForkNumber forknum)
{
int64 totalsize = 0;
- char *relationpath;
+ const char *relationpath;
char pathname[MAXPGPATH];
unsigned int segcount = 0;
@@ -753,7 +753,7 @@ pg_relation_filepath(PG_FUNCTION_ARGS)
Form_pg_class relform;
RelFileNode rnode;
BackendId backend;
- char *path;
+ const char *path;
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
if (!HeapTupleIsValid(tuple))
diff --git a/src/include/catalog/catalog.h b/src/include/catalog/catalog.h
index 678a945..d9036fe 100644
--- a/src/include/catalog/catalog.h
+++ b/src/include/catalog/catalog.h
@@ -31,7 +31,7 @@ extern const char *forkNames[];
extern ForkNumber forkname_to_number(char *forkName);
extern int forkname_chars(const char *str, ForkNumber *);
-extern char *relpathbackend(RelFileNode rnode, BackendId backend,
+extern const char *relpathbackend(RelFileNode rnode, BackendId backend,
ForkNumber forknum);
extern char *GetDatabasePath(Oid dbNode, Oid spcNode);
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h
index 940d9d4..8886db5 100644
--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -46,7 +46,7 @@
* FileSeek uses the standard UNIX lseek(2) flags.
*/
-typedef char *FileName;
+typedef const char *FileName;
typedef int File;
--
1.7.10.4
>From 9be062aa078ba02106fe4bd3f5429ca650332a36 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Tue, 11 Dec 2012 16:38:54 +0100
Subject: [PATCH 2/2] Remove empty pfree() definition from
pg_xlogdump/compat.c now its not needed anymore
---
src/bin/pg_xlogdump/compat.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/src/bin/pg_xlogdump/compat.c b/src/bin/pg_xlogdump/compat.c
index dd36e55..b9c242d 100644
--- a/src/bin/pg_xlogdump/compat.c
+++ b/src/bin/pg_xlogdump/compat.c
@@ -43,22 +43,16 @@ tliInHistory(TimeLineID tli, List *expectedTLEs)
return false;
}
-void
-pfree(void *a)
-{
-}
-
-
const char *
timestamptz_to_str(TimestampTz t)
{
return "";
}
-char *
+const char *
relpathbackend(RelFileNode rnode, BackendId backend, ForkNumber forknum)
{
- return NULL;
+ return "";
}
/*
--
1.7.10.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
