fix schema ownership on first connection preliminary patch v2

Dear hackers,

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.

(1) It adds a new "datisinit" attribute to pg_database, which tells
whether the database initialization was performed or not.
The documentation is updated accordingly.

(2) This boolean is tested in postinit.c:ReverifyMyDatabase,
and InitializeDatabase is called if necessary.

(3) The routine updates pg_database datisinit, as well as pg_namespace
ownership and acl stuff. The acl item update part is not yet
appropriate: I'd like some answers before going on.

(4) Some validation is added. It passes for me.

My questions:

- is the place for the first connection housekeeping updates appropriate?
it seems so from ReverifyMyDatabase comments, but these are only comments.

- it is quite convenient to use SPI_* stuff for this very rare updates,
but I'm not that confident about the issue... On the other hand, the
all-C solution would result in a much longer and less obvious code.

- it is unclear to me whether it should be allowed to skip this under
some condition, when the database is accessed in "debug" mode from
a standalone postgres for instance?

- also I'm wondering how to react (what to do and how to do it) on
errors within or under these initialization. For instance, how to
be sure that the CurrentUser is reset as expected?

Thanks in advance for your answers and comments.

Have a nice day.

--
Fabien Coelho - coelho(at)cri(dot)ensmp(dot)fr

I have added the v2 version of this patch to the patch queue (attached).
I agree with Tom that there is no need for regression tests for this
feature and have removed that part of the patch.

http://momjian.postgresql.org/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------

Fabien COELHO wrote:
>
> Dear hackers,
>
> 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.
>
> (1) It adds a new "datisinit" attribute to pg_database, which tells
> whether the database initialization was performed or not.
> The documentation is updated accordingly.
>
> (2) This boolean is tested in postinit.c:ReverifyMyDatabase,
> and InitializeDatabase is called if necessary.
>
> (3) The routine updates pg_database datisinit, as well as pg_namespace
> ownership and acl stuff. The acl item update part is not yet
> appropriate: I'd like some answers before going on.
>
> (4) Some validation is added. It passes for me.
>
>
> My questions:
>
> - is the place for the first connection housekeeping updates appropriate?
> it seems so from ReverifyMyDatabase comments, but these are only comments.
>
> - it is quite convenient to use SPI_* stuff for this very rare updates,
> but I'm not that confident about the issue... On the other hand, the
> all-C solution would result in a much longer and less obvious code.
>
> - it is unclear to me whether it should be allowed to skip this under
> some condition, when the database is accessed in "debug" mode from
> a standalone postgres for instance?
>
> - also I'm wondering how to react (what to do and how to do it) on
> errors within or under these initialization. For instance, how to
> be sure that the CurrentUser is reset as expected?
>
> Thanks in advance for your answers and comments.
>
> Have a nice day.
>
> --
> Fabien Coelho - coelho(at)cri(dot)ensmp(dot)fr

Content-Description:

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Tom is reviewing this.

---------------------------------------------------------------------------

Bruce Momjian wrote:
>
> I have added the v2 version of this patch to the patch queue (attached).
> I agree with Tom that there is no need for regression tests for this
> feature and have removed that part of the patch.
>
> http://momjian.postgresql.org/cgi-bin/pgpatches
>
> I will try to apply it within the next 48 hours.
>
> ---------------------------------------------------------------------------
>
> Fabien COELHO wrote:
> >
> > Dear hackers,
> >
> > 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.
> >
> > (1) It adds a new "datisinit" attribute to pg_database, which tells
> > whether the database initialization was performed or not.
> > The documentation is updated accordingly.
> >
> > (2) This boolean is tested in postinit.c:ReverifyMyDatabase,
> > and InitializeDatabase is called if necessary.
> >
> > (3) The routine updates pg_database datisinit, as well as pg_namespace
> > ownership and acl stuff. The acl item update part is not yet
> > appropriate: I'd like some answers before going on.
> >
> > (4) Some validation is added. It passes for me.
> >
> >
> > My questions:
> >
> > - is the place for the first connection housekeeping updates appropriate?
> > it seems so from ReverifyMyDatabase comments, but these are only comments.
> >
> > - it is quite convenient to use SPI_* stuff for this very rare updates,
> > but I'm not that confident about the issue... On the other hand, the
> > all-C solution would result in a much longer and less obvious code.
> >
> > - it is unclear to me whether it should be allowed to skip this under
> > some condition, when the database is accessed in "debug" mode from
> > a standalone postgres for instance?
> >
> > - also I'm wondering how to react (what to do and how to do it) on
> > errors within or under these initialization. For instance, how to
> > be sure that the CurrentUser is reset as expected?
> >
> > Thanks in advance for your answers and comments.
> >
> > Have a nice day.
> >
> > --
> > Fabien Coelho - coelho(at)cri(dot)ensmp(dot)fr
>
> Content-Description:
>
> [ Attachment, skipping... ]
>
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 7: don't forget to increase your free space map settings
>
> --
> Bruce Momjian | http://candle.pha.pa.us
> pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
> + If your life is a hard drive, | 13 Roberts Road
> + Christ can be your backup. | Newtown Square, Pennsylvania 19073

> Dear hackers,
>
> Please find attached a patch to fix schema ownership on first
> connection, so that non system schemas reflect the database owner.
>
>
> This revised patch includes fixes after Tom comments on names used in
> the validation. However, the validation is still there. It is easy to
> edit it out if required, but I still think that such a test is a good thing.
>
>
> (1) It adds a new "datisinit" attribute to pg_database, which tells
> whether the database initialization was performed or not.
> The documentation is updated accordingly.
>
>
> (2) The datisnit boolean is tested in postinit.c:ReverifyMyDatabase,
> and InitializeDatabase is called if necessary.
>
>
> (3) The routine updates pg_database datisinit, as well as pg_namespace
> ownership and acl stuff.
>
>
> I think that the race condition if two backends connect for
> the first time to the same database is correctly taken care of,
> as only one backend will update the datisinit flag and then will fix the
> schema ownership.
>
>
> (4) Some validation is added.
>
>
> Some questions:
>
> - is the place for the first connection housekeeping updates appropriate?
> it seems so from ReverifyMyDatabase comments, but these are only comments.
>
>
> - it is quite convenient to use SPI_* stuff for this very rare updates,
> but I'm not that confident about the issue... On the other hand, the
> all-C solution would result in a much longer and less obvious code:-(
>
>
> - it is unclear to me whether it should be allowed to skip this under
> some condition, when the database is accessed in "debug" mode from
> a standalone postgres for instance?
>
>
> - also I'm wondering how to react (what to do and how to do it) on
> errors within or under these initialization. For instance, how to
> be sure that the CurrentUser is reset as expected?
>
>
> Thanks in advance for any comments.
>
> Have a nice day.
>
> --
> Fabien.
>
> *** ./doc/src/sgml/catalogs.sgml.orig Mon Jun 7 09:08:11 2004
> --- ./doc/src/sgml/catalogs.sgml Wed Jun 9 10:26:39 2004
> ***************
> *** 1536,1541 ****
> --- 1536,1552 ----
> </row>
>
> <row>
> + <entry><structfield>datisinit</structfield></entry>
> + <entry><type>bool</type></entry>
> + <entry></entry>
> + <entry>
> + False when a database is just created but housekeeping initialization
> + tasks are not performed yet. On the first connection, the initialization
> + is performed and the boolean is turned to true.
> + </entry>
> + </row>
> +
> + <row>
> <entry><structfield>datlastsysoid</structfield></entry>
> <entry><type>oid</type></entry>
> <entry></entry>
> *** ./src/backend/commands/dbcommands.c.orig Wed May 26 17:28:40 2004
> --- ./src/backend/commands/dbcommands.c Wed Jun 9 10:26:39 2004
> ***************
> *** 424,429 ****
> --- 424,430 ----
> new_record[Anum_pg_database_encoding - 1] = Int32GetDatum(encoding);
> new_record[Anum_pg_database_datistemplate - 1] = BoolGetDatum(false);
> new_record[Anum_pg_database_datallowconn - 1] = BoolGetDatum(true);
> + new_record[Anum_pg_database_datisinit - 1] = BoolGetDatum(false);
> new_record[Anum_pg_database_datlastsysoid - 1] = ObjectIdGetDatum(src_lastsysoid);
> new_record[Anum_pg_database_datvacuumxid - 1] = TransactionIdGetDatum(src_vacuumxid);
> new_record[Anum_pg_database_datfrozenxid - 1] = TransactionIdGetDatum(src_frozenxid);
> *** ./src/backend/utils/adt/acl.c.orig Thu Jun 3 15:05:57 2004
> --- ./src/backend/utils/adt/acl.c Wed Jun 9 10:26:39 2004
> ***************
> *** 2203,2205 ****
> --- 2203,2225 ----
> errmsg("unrecognized privilege type: \"%s\"", priv_type)));
> return ACL_NO_RIGHTS; /* keep compiler quiet */
> }
> +
> + /* acl acl_switch_grantor(acl, oldgrantor, newgrantor);
> + * switch grantor id in aclitem array.
> + * used internally for fixing owner rights in new databases.
> + * must be STRICT.
> + */
> + Datum acl_switch_grantor(PG_FUNCTION_ARGS)
> + {
> + Acl * acls = PG_GETARG_ACL_P_COPY(0);
> + int i,
> + old_grantor = PG_GETARG_INT32(1),
> + new_grantor = PG_GETARG_INT32(2);
> + AclItem * item;
> +
> + for (i=0, item=ACL_DAT(acls); i<ACL_NUM(acls); i++, item++)
> + if (item->ai_grantor == old_grantor)
> + item->ai_grantor = new_grantor;
> +
> + PG_RETURN_ACL_P(acls);
> + }
> *** ./src/backend/utils/init/postinit.c.orig Tue Jun 1 10:21:23 2004
> --- ./src/backend/utils/init/postinit.c Wed Jun 9 11:52:02 2004
> ***************
> *** 50,55 ****
> --- 50,110 ----
>
> /*** InitPostgres support ***/
>
> + #include "executor/spi.h"
> +
> + /* Do housekeeping initializations if required, on first connection.
> + * This function is expected to be called from within a transaction block.
> + */
> + static void InitializeDatabase(const char * dbname)
> + {
> + /* su */
> + AclId saved_user = GetUserId();
> + SetUserId(1);
> +
> + /* Querying in C is nice, but SQL is nicer.
> + * This is only done once in a lifetime of the database,
> + * so paying for the parser/optimiser cost is not that bad?
> + * What if that fails?
> + */
> + SetQuerySnapshot();
> +
> + if (SPI_connect() != SPI_OK_CONNECT)
> + ereport(ERROR, (errmsg("SPI_connect failed")));
> +
> + if (SPI_exec("UPDATE " SystemCatalogName "." DatabaseRelationName
> + " SET datisinit=TRUE"
> + " WHERE datname=CURRENT_DATABASE()"
> + " AND datisinit=FALSE" , 0) != SPI_OK_UPDATE)
> + ereport(ERROR, (errmsg("database initialization %s update failed",
> + DatabaseRelationName)));
> +
> + if (SPI_processed==1)
> + {
> + /* ok, we have it! */
> +
> + if (SPI_exec("UPDATE " SystemCatalogName "." NamespaceRelationName
> + " SET nspowner=datdba,"
> + " nspacl = acl_switch_grantor(nspacl, 1, datdba)"
> + " FROM " SystemCatalogName "." DatabaseRelationName " "
> + " WHERE nspname NOT LIKE"
> + " ALL(ARRAY['pg_%','information_schema'])"
> + " AND datname=CURRENT_DATABASE()"
> + " AND nspowner!=datdba;", 0) != SPI_OK_UPDATE)
> + ereport(ERROR, (errmsg("database initialization %s update failed",
> + NamespaceRelationName)));
> +
> + if (SPI_processed>0)
> + ereport(LOG, /* don't bother the user about these details... */
> + (errmsg("database initialization schema owner updates: %d",
> + SPI_processed)));
> + }
> + /* some other concurrent connection did it, let us proceed. */
> +
> + if (SPI_finish() != SPI_OK_FINISH)
> + ereport(ERROR, (errmsg("SPI_finish failed")));
> +
> + SetUserId(saved_user);
> + }
>
> /* --------------------------------
> * ReverifyMyDatabase
> ***************
> *** 130,135 ****
> --- 185,196 ----
> errmsg("database \"%s\" is not currently accepting connections",
> name)));
>
> + /* Do we need the housekeeping initialization of the database?
> + * could be skipped on standalone "panic" mode?
> + */
> + if (!dbform->datisinit)
> + InitializeDatabase(name);
> +
> /*
> * OK, we're golden. Only other to-do item is to save the encoding
> * info out of the pg_database tuple.
> *** ./src/include/catalog/catname.h.orig Sat Nov 29 23:40:58 2003
> --- ./src/include/catalog/catname.h Wed Jun 9 10:26:39 2004
> ***************
> *** 14,19 ****
> --- 14,20 ----
> #ifndef CATNAME_H
> #define CATNAME_H
>
> + #define SystemCatalogName "pg_catalog"
>
> #define AggregateRelationName "pg_aggregate"
> #define AccessMethodRelationName "pg_am"
> *** ./src/include/catalog/catversion.h.orig Mon Jun 7 09:08:19 2004
> --- ./src/include/catalog/catversion.h Wed Jun 9 10:26:39 2004
> ***************
> *** 53,58 ****
> */
>
> /* yyyymmddN */
> ! #define CATALOG_VERSION_NO 200406061
>
> #endif
> --- 53,58 ----
> */
>
> /* yyyymmddN */
> ! #define CATALOG_VERSION_NO 200406081
>
> #endif
> *** ./src/include/catalog/pg_attribute.h.orig Mon Apr 5 12:06:43 2004
> --- ./src/include/catalog/pg_attribute.h Wed Jun 9 10:26:39 2004
> ***************
> *** 287,299 ****
> DATA(insert ( 1262 encoding 23 -1 4 3 0 -1 -1 t p i t f f t 0));
> DATA(insert ( 1262 datistemplate 16 -1 1 4 0 -1 -1 t p c t f f t 0));
> DATA(insert ( 1262 datallowconn 16 -1 1 5 0 -1 -1 t p c t f f t 0));
> ! DATA(insert ( 1262 datlastsysoid 26 -1 4 6 0 -1 -1 t p i t f f t 0));
> ! DATA(insert ( 1262 datvacuumxid 28 -1 4 7 0 -1 -1 t p i t f f t 0));
> ! DATA(insert ( 1262 datfrozenxid 28 -1 4 8 0 -1 -1 t p i t f f t 0));
> /* do not mark datpath as toastable; GetRawDatabaseInfo won't cope */
> ! DATA(insert ( 1262 datpath 25 -1 -1 9 0 -1 -1 f p i t f f t 0));
> ! DATA(insert ( 1262 datconfig 1009 -1 -1 10 1 -1 -1 f x i f f f t 0));
> ! DATA(insert ( 1262 datacl 1034 -1 -1 11 1 -1 -1 f x i f f f t 0));
> DATA(insert ( 1262 ctid 27 0 6 -1 0 -1 -1 f p s t f f t 0));
> DATA(insert ( 1262 oid 26 0 4 -2 0 -1 -1 t p i t f f t 0));
> DATA(insert ( 1262 xmin 28 0 4 -3 0 -1 -1 t p i t f f t 0));
> --- 287,300 ----
> DATA(insert ( 1262 encoding 23 -1 4 3 0 -1 -1 t p i t f f t 0));
> DATA(insert ( 1262 datistemplate 16 -1 1 4 0 -1 -1 t p c t f f t 0));
> DATA(insert ( 1262 datallowconn 16 -1 1 5 0 -1 -1 t p c t f f t 0));
> ! DATA(insert ( 1262 datisinit 16 -1 1 6 0 -1 -1 t p c t f f t 0));
> ! DATA(insert ( 1262 datlastsysoid 26 -1 4 7 0 -1 -1 t p i t f f t 0));
> ! DATA(insert ( 1262 datvacuumxid 28 -1 4 8 0 -1 -1 t p i t f f t 0));
> ! DATA(insert ( 1262 datfrozenxid 28 -1 4 9 0 -1 -1 t p i t f f t 0));
> /* do not mark datpath as toastable; GetRawDatabaseInfo won't cope */
> ! DATA(insert ( 1262 datpath 25 -1 -1 10 0 -1 -1 f p i t f f t 0));
> ! DATA(insert ( 1262 datconfig 1009 -1 -1 11 1 -1 -1 f x i f f f t 0));
> ! DATA(insert ( 1262 datacl 1034 -1 -1 12 1 -1 -1 f x i f f f t 0));
> DATA(insert ( 1262 ctid 27 0 6 -1 0 -1 -1 f p s t f f t 0));
> DATA(insert ( 1262 oid 26 0 4 -2 0 -1 -1 t p i t f f t 0));
> DATA(insert ( 1262 xmin 28 0 4 -3 0 -1 -1 t p i t f f t 0));
> *** ./src/include/catalog/pg_class.h.orig Mon Apr 5 12:06:43 2004
> --- ./src/include/catalog/pg_class.h Wed Jun 9 10:26:39 2004
> ***************
> *** 146,152 ****
> DESCR("");
> DATA(insert OID = 1261 ( pg_group PGNSP 87 PGUID 0 1261 0 0 0 0 f t r 3 0 0 0 0 0 f f f f _null_ ));
> DESCR("");
> ! DATA(insert OID = 1262 ( pg_database PGNSP 88 PGUID 0 1262 0 0 0 0 f t r 11 0 0 0 0 0 t f f f _null_ ));
> DESCR("");
> DATA(insert OID = 376 ( pg_xactlock PGNSP 0 PGUID 0 0 0 0 0 0 f t s 1 0 0 0 0 0 f f f f _null_ ));
> DESCR("");
> --- 146,152 ----
> DESCR("");
> DATA(insert OID = 1261 ( pg_group PGNSP 87 PGUID 0 1261 0 0 0 0 f t r 3 0 0 0 0 0 f f f f _null_ ));
> DESCR("");
> ! DATA(insert OID = 1262 ( pg_database PGNSP 88 PGUID 0 1262 0 0 0 0 f t r 12 0 0 0 0 0 t f f f _null_ ));
> DESCR("");
> DATA(insert OID = 376 ( pg_xactlock PGNSP 0 PGUID 0 0 0 0 0 0 f t s 1 0 0 0 0 0 f f f f _null_ ));
> DESCR("");
> *** ./src/include/catalog/pg_database.h.orig Tue Feb 10 02:55:26 2004
> --- ./src/include/catalog/pg_database.h Wed Jun 9 10:26:39 2004
> ***************
> *** 38,43 ****
> --- 38,44 ----
> int4 encoding; /* character encoding */
> bool datistemplate; /* allowed as CREATE DATABASE template? */
> bool datallowconn; /* new connections allowed? */
> + bool datisinit; /* was it already initialized? */
> Oid datlastsysoid; /* highest OID to consider a system OID */
> TransactionId datvacuumxid; /* all XIDs before this are vacuumed */
> TransactionId datfrozenxid; /* all XIDs before this are frozen */
> ***************
> *** 57,76 ****
> * compiler constants for pg_database
> * ----------------
> */
> ! #define Natts_pg_database 11
> #define Anum_pg_database_datname 1
> #define Anum_pg_database_datdba 2
> #define Anum_pg_database_encoding 3
> #define Anum_pg_database_datistemplate 4
> #define Anum_pg_database_datallowconn 5
> ! #define Anum_pg_database_datlastsysoid 6
> ! #define Anum_pg_database_datvacuumxid 7
> ! #define Anum_pg_database_datfrozenxid 8
> ! #define Anum_pg_database_datpath 9
> ! #define Anum_pg_database_datconfig 10
> ! #define Anum_pg_database_datacl 11
>
> ! DATA(insert OID = 1 ( template1 PGUID ENCODING t t 0 0 0 "" _null_ _null_ ));
> DESCR("Default template database");
> #define TemplateDbOid 1
>
> --- 58,78 ----
> * compiler constants for pg_database
> * ----------------
> */
> ! #define Natts_pg_database 12
> #define Anum_pg_database_datname 1
> #define Anum_pg_database_datdba 2
> #define Anum_pg_database_encoding 3
> #define Anum_pg_database_datistemplate 4
> #define Anum_pg_database_datallowconn 5
> ! #define Anum_pg_database_datisinit 6
> ! #define Anum_pg_database_datlastsysoid 7
> ! #define Anum_pg_database_datvacuumxid 8
> ! #define Anum_pg_database_datfrozenxid 9
> ! #define Anum_pg_database_datpath 10
> ! #define Anum_pg_database_datconfig 11
> ! #define Anum_pg_database_datacl 12
>
> ! DATA(insert OID = 1 ( template1 PGUID ENCODING t t t 0 0 0 "" _null_ _null_ ));
> DESCR("Default template database");
> #define TemplateDbOid 1
>
> *** ./src/include/catalog/pg_proc.h.orig Mon Jun 7 09:08:20 2004
> --- ./src/include/catalog/pg_proc.h Wed Jun 9 10:26:39 2004
> ***************
> *** 3588,3593 ****
> --- 3588,3596 ----
> DATA(insert OID = 2243 ( bit_or PGNSP PGUID 12 t f f f i 1 1560 "1560" _null_ aggregate_dummy - _null_));
> DESCR("bitwise-or bit aggregate");
>
> + DATA(insert OID = 2245 ( acl_switch_grantor PGNSP PGUID 12 f f t f i 3 1034 "1034 23 23" _null_ acl_switch_grantor - _null_));
> + DESCR("internal function to update grantors in acls");
> +
> /*
> * Symbolic values for provolatile column: these indicate whether the result
> * of a function is dependent *only* on the values of its explicit arguments,
> *** ./src/include/utils/acl.h.orig Thu Jun 3 15:05:58 2004
> --- ./src/include/utils/acl.h Wed Jun 9 10:26:39 2004
> ***************
> *** 236,241 ****
> --- 236,242 ----
> extern Datum makeaclitem(PG_FUNCTION_ARGS);
> extern Datum aclitem_eq(PG_FUNCTION_ARGS);
> extern Datum hash_aclitem(PG_FUNCTION_ARGS);
> + extern Datum acl_switch_grantor(PG_FUNCTION_ARGS);
>
> /*
> * prototypes for functions in aclchk.c

>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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

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

In my batch of pg_dump patches that have gone in, pg_dump will now do
exactly that. The reason was so that if someone altered the owner of
their public schema, it should be dumped like that.

Chris

Is there a TODO here?

---------------------------------------------------------------------------

Tom Lane wrote:
> 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
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

> > 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 :-(

No need to apoligize.

> In general I do not like the approach of using SPI for this. [...]

Ok.

Well, my actual motivation for using SPI is to write 3 lines of SQL were
the intent is obvious instead of 60 lines of obscure C. I'm just lazy.

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

Well, I did not noticed this good function.

> I do not like the hardwired assumption that userID 1 exists and is
> a superuser.

With UNIX, there is a pretty hard assumption that root is number 0, and I
assumed - wrongly - that the same line of reasonning applied to pg.

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

My implicit assumption was that if it is not 1, it means that someone
decide to give some template1 schemas to someone, and I don't have to
change that.

> However, enough about implementation deficiencies.

Anyway, it is good to know.

> Did we really have consensus that the system should do this in the first
> place?

I'm convinced that the current status is not appropriate, but that does
not mean that what the patch suggests a good fix.

The current status is that when you do "CREATE DATABASE foo WITH OWNER
calvin", user calvin which "owns" the database is prevented from
manipulating the "public" schema...

The underlying question is "what is a database owner?". My feeling is that
it is like a unix-directory owner, that is someone who can do whatever it
likes at home, but cannot change the system (/bin, /lib).

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

Mmm... my assumption here was that either it is a "system" schema, and it
is special, or it is not, and then it is not special. But it is only an
assumption, which makes sense with the "unix-like" owner approach.

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

Ok. Thanks for your comments.

--
Fabien Coelho - coelho(at)cri(dot)ensmp(dot)fr

Dear Bruce,

> Is there a TODO here?

I think yes. I would suggest an open entry like:

. decide what a "database owner" is expected to be able do (esp. wrt acls)
on non-system schema, e.g. "public", and then implement it.
- should the database owner own the "public" schema by default?
- what about other non-system schemas?
- what about other objects?

ISTM that it reflects that although the "what" is unclear, there is an
issue somewhere;-)

--
Fabien Coelho - coelho(at)cri(dot)ensmp(dot)fr

Added to TODO:

* Set proper permissions on non-system schemas during db creation

Currently all schemas are owned by the super-user because they are
copied from the template1 database.

---------------------------------------------------------------------------

Fabien COELHO wrote:
>
> Dear Bruce,
>
> > Is there a TODO here?
>
> I think yes. I would suggest an open entry like:
>
> . decide what a "database owner" is expected to be able do (esp. wrt acls)
> on non-system schema, e.g. "public", and then implement it.
> - should the database owner own the "public" schema by default?
> - what about other non-system schemas?
> - what about other objects?
>
> ISTM that it reflects that although the "what" is unclear, there is an
> issue somewhere;-)
>
> --
> Fabien Coelho - coelho(at)cri(dot)ensmp(dot)fr
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073