Re: %d in log_line_prefix doesn't work for bg/autovacuum workers

Hi,

On 2014-05-17 12:23:51 -0400, Tom Lane wrote:
> 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.

I think it was just about making it harder to hit issues related to
renames. But since there wasn't enough infrastructure to make it
bulletproof yet...

>  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.

It's 52667d56a3b489e5645f069522631824b7ffc520. I've lost the initial 5
when copying it into emacs.

> 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 don't see much need to allow it. But even if we're interested in
allowing rename properly there'll definitely be the need to update the
database name used in log messages. This doesn't seem to make it
harder. My guess would be that we'd need some kind of invalidation
message for it...

> > 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.

Hm. I tentatively think that it's still reasonable to report it
earlier. There's a bunch of things (option processing, authentication)
that can error out before the database name is finally validated. It
seems somewhat helpful to give context there.
It's easy enough to get data from connecting clients into the log
anyway, so there doesn't seem to be a a security argument either.

> I'd personally fill MyDatabaseName
> a bit sooner than you have here, probably about where MyProc->databaseId
> gets set.

The reason I placed it where I did is that it's the first location where
we can be sure the database conected to is the correct one because now
the lock on the database is held and we've rechecked the name.

> But I don't think elog.c should be looking at more than one source of
> truth for %d.

Ok.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services