On 2014-05-17 00:17:46 +0200, Andres Freund wrote:
> On 2014-05-16 17:48:28 -0400, Tom Lane wrote:
> > This is basically reintroducing a variable that used to exist and was
> > intentionally gotten rid of, on the grounds that it'd fail to track
> > any renaming of the session's current database (cf commit b256f24264).
> > I'm not sure how much effort ought to be put into dealing with that corner
> > case; but let's not reintroduce old bugs just for failure to remember
> > them.
>
> I did look whether there's a race making MyDatabaseName out of date. I
> didn't see any. There's a windows where it could be renamed between
> where I'd done the MyDatabaseName = ... and the LockSharedObject() a few
> lines below. But directly afterwards there's a check that the database name is
> still correct.
> We could move the filling of MyDatabaseName to lessen the amount of
> comments that need to be added.
Moving it also provides the name in bootstrap mode. No idea whether
there'll ever be a use in that but given the variable may later replace
some get_database_name(MyDatabaseId) calls it seems to be a benefit.
I'd briefly changed elog.c to only use MydatabaseName, thinking that
there seems to be little reason to continue using database_name in
elog.c once the variable is introduced. But that's not a good idea:
MyProcPort->database_name exists earlier.
It's imo a bit debatable whether it's a good idea to log database names
in log_line_prefix that don't exist. But that's a separate change.
I've changed it though so that MyDatabaseName is preferred over
MyProc. It's imo easier to see that it has the correct value. And it'll
be correct if we ever support mapping authentication and real database
names or something.
Revised patch attached.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 3dd4063e607b0800fe74d33188c3198961ada073 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Fri, 16 May 2014 22:34:47 +0200
Subject: [PATCH] Support showing the database in log entries issued by
background processes.
Until now %d in log_line_prefix and the database_name column in csvlog
didn't display the database for autovacuum and background worker
processes. Because logging should better not perform catalog accesses
MyProcPort->database_name was used. The database name specified by the
user when connecting. As that structure isn't used in background
processes that aren't connected to clients log messages where using an
empty/NULL database name respectively..
Add a new global variable MyDabaseName that contains the database name
of the database a processes is connected to. Such a variable even used
to exist, back in b256f24264, but it was removed due to concerns about
it being out of date because of database renames. But since then
2667d56a3b added locking that prevents a database with connected users
from being renamed.
---
src/backend/utils/error/elog.c | 13 ++++++++++---
src/backend/utils/init/globals.c | 1 +
src/backend/utils/init/postinit.c | 11 +++++++++++
src/include/miscadmin.h | 1 +
4 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 0d92dcd..41b93f3 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2320,9 +2320,14 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
padding > 0 ? padding : -padding);
break;
case 'd':
- if (MyProcPort)
+ if (MyProcPort || MyDatabaseName)
{
- const char *dbname = MyProcPort->database_name;
+ const char *dbname;
+
+ if (MyDatabaseName)
+ dbname = MyDatabaseName;
+ else
+ dbname = MyProcPort->database_name;
if (dbname == NULL || *dbname == '\0')
dbname = _("[unknown]");
@@ -2575,7 +2580,9 @@ write_csvlog(ErrorData *edata)
appendStringInfoChar(&buf, ',');
/* database name */
- if (MyProcPort)
+ if (MyDatabaseName)
+ appendCSVLiteral(&buf, MyDatabaseName);
+ else if (MyProcPort)
appendCSVLiteral(&buf, MyProcPort->database_name);
appendStringInfoChar(&buf, ',');
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index be74835..46d4cf4 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -62,6 +62,7 @@ char postgres_exec_path[MAXPGPATH]; /* full path to backend */
BackendId MyBackendId = InvalidBackendId;
Oid MyDatabaseId = InvalidOid;
+char *MyDatabaseName = NULL;
Oid MyDatabaseTableSpace = InvalidOid;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index ed936d7..ca11ae4 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -51,6 +51,7 @@
#include "utils/acl.h"
#include "utils/fmgroids.h"
#include "utils/guc.h"
+#include "utils/memutils.h"
#include "utils/pg_locale.h"
#include "utils/portal.h"
#include "utils/ps_status.h"
@@ -862,6 +863,16 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
}
/*
+ * We can now safely store the database name without fear of it going
+ * stale. The locking of the database above, combined with the recheck
+ * that the name's still good, provides safety against a concurrent RENAME
+ * DATABASE. RenameDatabase() first acquires an exclusive lock on the
+ * database and only proceeds with renaming after checking whether there
+ * are any users of the database.
+ */
+ MyDatabaseName = MemoryContextStrdup(TopMemoryContext, dbname);
+
+ /*
* Now we should be able to access the database directory safely. Verify
* it's there and looks reasonable.
*/
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index c2b786e..91710d3 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -165,6 +165,7 @@ extern char postgres_exec_path[];
* extern BackendId MyBackendId;
*/
extern PGDLLIMPORT Oid MyDatabaseId;
+extern PGDLLIMPORT char *MyDatabaseName;
extern PGDLLIMPORT Oid MyDatabaseTableSpace;
--
2.0.0.rc2.4.g1dc51c6.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
