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 - [EMAIL PROTECTED] > > 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 > [EMAIL PROTECTED] | (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 [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 ---------------------------(end of broadcast)--------------------------- TIP 7: don't forget to increase your free space map settings
