Re: invalid search_path complaints - Mailing list pgsql-hackers

Christoph Berg <cb@df7cb.de> writes:
> Re: Tom Lane 2012-04-04 <28647.1333558029@sss.pgh.pa.us>
>> Now, Scott's comment seems to me to offer a principled way out of this:
>> if we define the intended semantics of search_path as being similar
>> to the traditional understanding of Unix PATH, then it's not an error
>> or even unexpected to have references to nonexistent schemas in there.

> Btw, the default setting does already work like this: "$user",public.
> It is not an error for "$user" not to exist, but it is a very nice
> default because it will be used as soon as it appears.

Yeah.  Between that and the fact that there are a lot of cases where we
simply fail to check path validity at all (eg, if it's coming from
postgresql.conf), I'm becoming more and more convinced that just
removing the existence check is the best thing.

Attached is a proposed patch for this.  (Note: the docs delta includes
mention of permissions behavior, which was previously undocumented but
has not actually changed.)

I am not sure whether we should consider back-patching this into 9.1,
although that would be necessary if we wanted to fix Robert's original
complaint against 9.1.  Thoughts?

            regards, tom lane

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 640defde860d57a81d0671f2957b99ded15a3566..361ad7b99a52bbbcec570b639800c175d3c19ab7 100644
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*************** COPY postgres_log FROM '/full/path/to/lo
*** 4670,4679 ****

         <para>
          The value for <varname>search_path</varname> must be a comma-separated
!         list of schema names.  If one of the list items is
!         the special value <literal>$user</literal>, then the schema
!         having the name returned by <function>SESSION_USER</> is substituted, if there
!         is such a schema.  (If not, <literal>$user</literal> is ignored.)
         </para>

         <para>
--- 4670,4686 ----

         <para>
          The value for <varname>search_path</varname> must be a comma-separated
!         list of schema names.  Any name that is not an existing schema, or is
!         a schema for which the user does not have <literal>USAGE</>
!         permission, is silently ignored.
!        </para>
!
!        <para>
!         If one of the list items is the special name
!         <literal>$user</literal>, then the schema having the name returned by
!         <function>SESSION_USER</> is substituted, if there is such a schema
!         and the user has <literal>USAGE</> permission for it.
!         (If not, <literal>$user</literal> is ignored.)
         </para>

         <para>
*************** COPY postgres_log FROM '/full/path/to/lo
*** 4697,4712 ****

         <para>
          When objects are created without specifying a particular target
!         schema, they will be placed in the first schema listed
!         in the search path.  An error is reported if the search path is
!         empty.
         </para>

         <para>
          The default value for this parameter is
!         <literal>'"$user", public'</literal> (where the second part will be
!         ignored if there is no schema named <literal>public</>).
!         This supports shared use of a database (where no users
          have private schemas, and all share use of <literal>public</>),
          private per-user schemas, and combinations of these.  Other
          effects can be obtained by altering the default search path
--- 4704,4718 ----

         <para>
          When objects are created without specifying a particular target
!         schema, they will be placed in the first valid schema named in
!         <varname>search_path</varname>.  An error is reported if the search
!         path is empty.
         </para>

         <para>
          The default value for this parameter is
!         <literal>"$user", public</literal>.
!         This setting supports shared use of a database (where no users
          have private schemas, and all share use of <literal>public</>),
          private per-user schemas, and combinations of these.  Other
          effects can be obtained by altering the default search path
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index dc8f8eaf3f3f60f51fd8b59aa78ccfc36e1b23f9..e92efd863ed74fb77425333d772c194c3d36851b 100644
*** a/src/backend/catalog/namespace.c
--- b/src/backend/catalog/namespace.c
*************** ResetTempTableNamespace(void)
*** 3773,3786 ****
   * Routines for handling the GUC variable 'search_path'.
   */

! /* check_hook: validate new search_path, if possible */
  bool
  check_search_path(char **newval, void **extra, GucSource source)
  {
-     bool        result = true;
      char       *rawname;
      List       *namelist;
-     ListCell   *l;

      /* Need a modifiable copy of string */
      rawname = pstrdup(*newval);
--- 3773,3784 ----
   * Routines for handling the GUC variable 'search_path'.
   */

! /* check_hook: validate new search_path value */
  bool
  check_search_path(char **newval, void **extra, GucSource source)
  {
      char       *rawname;
      List       *namelist;

      /* Need a modifiable copy of string */
      rawname = pstrdup(*newval);
*************** check_search_path(char **newval, void **
*** 3796,3847 ****
      }

      /*
!      * If we aren't inside a transaction, we cannot do database access so
!      * cannot verify the individual names.    Must accept the list on faith.
       */
-     if (IsTransactionState())
-     {
-         /*
-          * Verify that all the names are either valid namespace names or
-          * "$user" or "pg_temp".  We do not require $user to correspond to a
-          * valid namespace, and pg_temp might not exist yet.  We do not check
-          * for USAGE rights, either; should we?
-          *
-          * When source == PGC_S_TEST, we are checking the argument of an ALTER
-          * DATABASE SET or ALTER USER SET command.    It could be that the
-          * intended use of the search path is for some other database, so we
-          * should not error out if it mentions schemas not present in the
-          * current database.  We issue a NOTICE instead.
-          */
-         foreach(l, namelist)
-         {
-             char       *curname = (char *) lfirst(l);
-
-             if (strcmp(curname, "$user") == 0)
-                 continue;
-             if (strcmp(curname, "pg_temp") == 0)
-                 continue;
-             if (!SearchSysCacheExists1(NAMESPACENAME,
-                                        CStringGetDatum(curname)))
-             {
-                 if (source == PGC_S_TEST)
-                     ereport(NOTICE,
-                             (errcode(ERRCODE_UNDEFINED_SCHEMA),
-                            errmsg("schema \"%s\" does not exist", curname)));
-                 else
-                 {
-                     GUC_check_errdetail("schema \"%s\" does not exist", curname);
-                     result = false;
-                     break;
-                 }
-             }
-         }
-     }

      pfree(rawname);
      list_free(namelist);

!     return result;
  }

  /* assign_hook: do extra actions as needed */
--- 3794,3810 ----
      }

      /*
!      * We used to try to check that the named schemas exist, but there are
!      * many valid use-cases for having search_path settings that include
!      * schemas that don't exist; and often, we are not inside a transaction
!      * here and so can't consult the system catalogs anyway.  So now, the only
!      * requirement is syntactic validity of the identifier list.
       */

      pfree(rawname);
      list_free(namelist);

!     return true;
  }

  /* assign_hook: do extra actions as needed */
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index d324862049955574a9fda42799e6cb66248688d8..271706d31e9301147ffa85065ff260db2b60730d 100644
*** a/src/test/regress/expected/guc.out
--- b/src/test/regress/expected/guc.out
*************** SELECT current_user = 'temp_reset_user';
*** 605,610 ****
--- 605,635 ----

  DROP ROLE temp_reset_user;
  --
+ -- search_path should react to changes in pg_namespace
+ --
+ set search_path = foo, public, not_there_initially;
+ select current_schemas(false);
+  current_schemas
+ -----------------
+  {public}
+ (1 row)
+
+ create schema not_there_initially;
+ select current_schemas(false);
+        current_schemas
+ ------------------------------
+  {public,not_there_initially}
+ (1 row)
+
+ drop schema not_there_initially;
+ select current_schemas(false);
+  current_schemas
+ -----------------
+  {public}
+ (1 row)
+
+ reset search_path;
+ --
  -- Tests for function-local GUC settings
  --
  set work_mem = '3MB';
*************** select report_guc('work_mem'), current_s
*** 617,630 ****
   1MB        | 3MB
  (1 row)

! -- this should draw only a warning
! alter function report_guc(text) set search_path = no_such_schema;
! NOTICE:  schema "no_such_schema" does not exist
! -- with error occurring here
! select report_guc('work_mem'), current_setting('work_mem');
! ERROR:  invalid value for parameter "search_path": "no_such_schema"
! DETAIL:  schema "no_such_schema" does not exist
! alter function report_guc(text) reset search_path set work_mem = '2MB';
  select report_guc('work_mem'), current_setting('work_mem');
   report_guc | current_setting
  ------------+-----------------
--- 642,648 ----
   1MB        | 3MB
  (1 row)

! alter function report_guc(text) set work_mem = '2MB';
  select report_guc('work_mem'), current_setting('work_mem');
   report_guc | current_setting
  ------------+-----------------
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index 21ed86f26ba259e7258e6b69afa8f8e80e2a2442..0c217923814893d0aacc2c83d548fe778eeaded7 100644
*** a/src/test/regress/sql/guc.sql
--- b/src/test/regress/sql/guc.sql
*************** SELECT current_user = 'temp_reset_user';
*** 183,188 ****
--- 183,200 ----
  DROP ROLE temp_reset_user;

  --
+ -- search_path should react to changes in pg_namespace
+ --
+
+ set search_path = foo, public, not_there_initially;
+ select current_schemas(false);
+ create schema not_there_initially;
+ select current_schemas(false);
+ drop schema not_there_initially;
+ select current_schemas(false);
+ reset search_path;
+
+ --
  -- Tests for function-local GUC settings
  --

*************** set work_mem = '1MB';
*** 194,206 ****

  select report_guc('work_mem'), current_setting('work_mem');

! -- this should draw only a warning
! alter function report_guc(text) set search_path = no_such_schema;
!
! -- with error occurring here
! select report_guc('work_mem'), current_setting('work_mem');
!
! alter function report_guc(text) reset search_path set work_mem = '2MB';

  select report_guc('work_mem'), current_setting('work_mem');

--- 206,212 ----

  select report_guc('work_mem'), current_setting('work_mem');

! alter function report_guc(text) set work_mem = '2MB';

  select report_guc('work_mem'), current_setting('work_mem');