fix schema ownership on first connection preliminary patch v2 - Mailing list pgsql-patches

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