Re: fix schema ownership on first connection preliminary patch v2

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Fabien COELHO wrote:
>> Please find attached a preliminary patch to fix schema ownership on first
>> connection. It is for comments and advices as I still have doubts about
>> various how-and-where issues, thus it is not submitted to the patch list.

> I have added the v2 version of this patch to the patch queue (attached).

I do apologize for not having looked at this patch sooner, but it's been
at the bottom of the priority queue :-(

In general I do not like the approach of using SPI for this, because
it has to execute before the system is really quite fully up. Just
looking at InitPostgres, I note that the namespace search path isn't
set yet, for example, and I'm not real sure what that implies for
execution of these queries. I'm also uncomfortable with the fact that
RelationCacheInitializePhase3 isn't done yet --- the SPI execution could
pollute the relcache with stuff we don't really want written into
the relcache cache file. (Much of this could be dodged by having
ReverifyMyDatabase just pass back the datinit flag, and then taking
action on it (if any) at the bottom of InitPostgres instead of where
the patch puts it. But I'm still against using SPI for it.)

Also, since we already have AlterSchemaOwner coded at the C level,
the original rationale of not wanting to add code has been rendered
moot.

I do not like the hardwired assumption that userID 1 exists and is
a superuser. The code is also broken to assume that ID 1 is the owner
of the public schema in the source database (though this part is at
least easy to fix, and would go away anyway if using AlterSchemaOwner).

Lastly, I'm unconvinced that the (lack of) locking is safe. Two
backends trying to connect at the same time would make conflicting
attempts to UPDATE pg_database, and if the default transaction isolation
mode is SERIALIZABLE then one of them is going to get an actual failure,
not just decide it has nothing to do. A possible alternative is to take
out ExclusiveLock on pg_namespace before re-examining pg_database.

However, enough about implementation deficiencies. Did we really have
consensus that the system should do this in the first place? I was
only about halfway sold on the notion of changing public's ownership.
I especially dislike the detail that it will alter the ownership of
*all* non-builtin schemas, not only "public". If someone has added
special-purpose schemas to template1, it seems unlikely that this is
the behavior they'd want.

I'm also wondering about what side-effects this will have on pg_dump
behavior. In particular, will pg_dump try to "ALTER OWNER public",
and if so will that be appropriate? We haven't previously needed to
assume that we are restoring into a database with the same datowner as
we dumped from...

I think we should leave the behavior alone, at least for this release
cycle.

regards, tom lane