Re: fix schema ownership on first connection preliminary patch

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