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@cri.ensmp.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@candle.pha.pa.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@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073