Andres Freund <andres@2ndquadrant.com> writes:
> 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 did look whether there's a race making MyDatabaseName out of date. I
>> didn't see any.
Hm. Actually, it looks like commit b256f24264 was a bit schizophrenic:
although it removed this particular way in which a database rename could
cause trouble, it nonetheless *added* checks that there were no active
sessions in the database. As you say, the checks were race-prone at the
time and have since been made bulletproof (though I could not find the
commit you mentioned?). But the intent was there.
I think the key issue comes down to this comment in RenameDatabase:
* XXX Client applications probably store the current database somewhere, * so renaming it could cause confusion.
On the other hand, there may not * be an actual problem besides a little confusion, so think about this * and
decide.
If we're willing to decide that we will never support renaming an active
database, then the approach you're proposing makes sense. And it seems
to me that this issue of potentially confusing client apps is much the
strongest argument for making such a decision. But is it strong enough?
Given that there haven't been complaints in the past ten years about how
you can't rename an active database, I'm OK personally with locking this
down forever. But I wonder if anyone wants to argue the contrary.
> 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.
If we do this at all, I think actually that is a good idea.
MyProcPort->database_name is a *requested* db name, but arguably the
%d log line field should represent the *actual* connected db name,
which means it shouldn't start being reported until we have some
confidence that such a DB exists. I'd personally fill MyDatabaseName
a bit sooner than you have here, probably about where MyProc->databaseId
gets set. But I don't think elog.c should be looking at more than one
source of truth for %d.
regards, tom lane