Thread: invalid search_path complaints

If you use ALTER ROLE/DATABASE SET to configure an invalid
search_path, PostgreSQL 9.1 issues a complaint about the invalid
setting on each new connection.  This is a behavior change relatively
to previous releases, which did not.  git bisect blames this commit:

2594cf0e8c04406ffff19b1651c5a406d376657c is the first bad commit
commit 2594cf0e8c04406ffff19b1651c5a406d376657c
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Thu Apr 7 00:11:01 2011 -0400
  Revise the API for GUC variable assign hooks.
  The previous functions of assign hooks are now split between check hooks  and assign hooks, where the former can fail
butthe latter shouldn't.  Aside from being conceptually clearer, this approach exposes the  "canonicalized" form of the
variablevalue to guc.c without having to do  an actual assignment.  And that lets us fix the problem recently noted by
BerndHelmle that the auto-tune patch for wal_buffers resulted in bogus  log messages about "parameter "wal_buffers"
cannotbe changed without  restarting the server".  There may be some speed advantage too, because  this design lets
hookfunctions avoid re-parsing variable values when  restoring a previous state after a rollback (they can store a
pre-parsed representation of the value instead).  This patch also resolves a  longstanding annoyance about custom error
messagesfrom variable assign  hooks: they should modify, not appear separately from, guc.c's own message  about
"invalidparameter value".
 

Is this an intentional change?  And is it desirable?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> If you use ALTER ROLE/DATABASE SET to configure an invalid
> search_path, PostgreSQL 9.1 issues a complaint about the invalid
> setting on each new connection.  This is a behavior change relatively
> to previous releases, which did not.

I would say that's an improvement.  Do you think it isn't?

The real issue I think is not so much whether a warning appears, as
that prior releases actually installed the broken search_path value
as active, which seems worse than leaving the default in place.
        regards, tom lane

On Tue, Apr 3, 2012 at 11:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> If you use ALTER ROLE/DATABASE SET to configure an invalid
>> search_path, PostgreSQL 9.1 issues a complaint about the invalid
>> setting on each new connection.  This is a behavior change relatively
>> to previous releases, which did not.
>
> I would say that's an improvement.  Do you think it isn't?

It seems like a log spam hazard at high connection rates.

> The real issue I think is not so much whether a warning appears, as
> that prior releases actually installed the broken search_path value
> as active, which seems worse than leaving the default in place.

I don't have enough experience with broken search_path settings to
have a very certain opinion on this part one way or the other.
Generally I try not to run with broken configurations in the first
place, but it obviously does happen in real life.

But, trying to speculate... if your search path is busted because one
constituent schema has dropped and all the others are still there, you
might still want to still search the remaining schemas.  If your
search path is going to end up being completely empty, then you might
prefer to get whatever the underlying default is... or you might not.
Did you get the wrong search_path because it should have been set
per-user-and-database and was instead set per-user?  Or did you get
the wrong search_path because you connected to a database that wasn't
the one you intended to connect to?  I'm not sure there's a right
answer in this case.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Apr 3, 2012 at 12:05 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Apr 3, 2012 at 11:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> If you use ALTER ROLE/DATABASE SET to configure an invalid
>>> search_path, PostgreSQL 9.1 issues a complaint about the invalid
>>> setting on each new connection.  This is a behavior change relatively
>>> to previous releases, which did not.
>>
>> I would say that's an improvement.  Do you think it isn't?
>
> It seems like a log spam hazard at high connection rates.

In particular, you also get a warning in the log every time an autovac
worker starts up.  :-(

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 3, 2012 at 12:05 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Tue, Apr 3, 2012 at 11:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I would say that's an improvement. �Do you think it isn't?

>> It seems like a log spam hazard at high connection rates.

[ shrug... ]  Failing to report a problem is a problem, too.  By your
argument any error message anywhere should be removed because it might
be a "log spam hazard" at high reporting rates.
        regards, tom lane

On 3 April 2012 19:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Apr 3, 2012 at 12:05 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Tue, Apr 3, 2012 at 11:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> I would say that's an improvement.  Do you think it isn't?
>
>>> It seems like a log spam hazard at high connection rates.
>
> [ shrug... ]  Failing to report a problem is a problem, too.  By your
> argument any error message anywhere should be removed because it might
> be a "log spam hazard" at high reporting rates.

Surely there's a way to throttle it appropriately?

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On Tue, Apr 3, 2012 at 2:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Apr 3, 2012 at 12:05 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Tue, Apr 3, 2012 at 11:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> I would say that's an improvement.  Do you think it isn't?
>
>>> It seems like a log spam hazard at high connection rates.
>
> [ shrug... ]  Failing to report a problem is a problem, too.  By your
> argument any error message anywhere should be removed because it might
> be a "log spam hazard" at high reporting rates.

That seems rather reductio ad absurdum.  I mean, any error message
will repeat if the underlying condition repeats: for example, if there
are many attempts to read bad blocks from the disk, then you will get
many read errors.  But in this case, you get many errors even though
nothing new has happened, which as I understand is something we try to
avoid.  For example, when we deprecated the use of => as an operator
name, there was some confusion over whether we were going to warn when
the operator was defined or when it was used, and there was universal
consensus in favor of warning only about the former:

http://archives.postgresql.org/pgsql-hackers/2010-06/msg00493.php

So we have an established precedent that it is right to warn about
things that are sketchy at the time that they are defined, but not
every time they are used.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> So we have an established precedent that it is right to warn about
> things that are sketchy at the time that they are defined, but not
> every time they are used.

Sure, but we don't have that option available to us here --- or more
accurately, ALTER USER/DATABASE SET *does* warn if the search_path value
looks like it might be invalid according to the current context, but
that helps little for this problem.  What's important is whether the
value is valid when we attempt to apply it.

Basically, I don't think you've made a strong case for changing this
behavior; nor have you explained what you think we should do instead.
        regards, tom lane

Scott Mead <scottm@openscg.com> writes:
> Personally, I feel that if unix will let you be stupid:
>     $ export PATH=/usr/bin:/this/invalid/crazy/path
>     $ echo $PATH
>     /usr/bin:/this/invalid/crazy/path
> PG should trust that I'll get where I'm going eventually :)

Well, that's an interesting analogy.  Are you arguing that we should
always accept any syntactically-valid search_path setting, no matter
whether the mentioned schemas exist?  It wouldn't be hard to do that.
The fun stuff comes in when you try to say "I want a warning in these
contexts but not those", because (a) the behavior you think you want
turns out to be pretty squishy, and (b) it's not always clear from the
implementation level what the context is.
        regards, tom lane

Scott Mead <scottm@openscg.com> writes:
> On Wed, Apr 4, 2012 at 12:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Well, that's an interesting analogy.  Are you arguing that we should
>> always accept any syntactically-valid search_path setting, no matter
>> whether the mentioned schemas exist?  It wouldn't be hard to do that.

>    I think we should always accept a syntactically valid search_path.

I could live with that.

>> The fun stuff comes in when you try to say "I want a warning in these
>> contexts but not those", because (a) the behavior you think you want
>> turns out to be pretty squishy, and (b) it's not always clear from the
>> implementation level what the context is.

> ISTM that just issuing a warning whenever you set the search_path (no
> matter which context) feels valid (and better than the above *nix
> behavior).  I would personally be opposed to seeing it on login however.

You're getting squishy on me ...
        regards, tom lane

On Wed, Apr 4, 2012 at 12:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Scott Mead <scottm@openscg.com> writes:
>> On Wed, Apr 4, 2012 at 12:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Well, that's an interesting analogy.  Are you arguing that we should
>>> always accept any syntactically-valid search_path setting, no matter
>>> whether the mentioned schemas exist?  It wouldn't be hard to do that.
>
>>    I think we should always accept a syntactically valid search_path.
>
> I could live with that.
>
>>> The fun stuff comes in when you try to say "I want a warning in these
>>> contexts but not those", because (a) the behavior you think you want
>>> turns out to be pretty squishy, and (b) it's not always clear from the
>>> implementation level what the context is.
>
>> ISTM that just issuing a warning whenever you set the search_path (no
>> matter which context) feels valid (and better than the above *nix
>> behavior).  I would personally be opposed to seeing it on login however.
>
> You're getting squishy on me ...

My feeling on this is that it's OK to warn if the search_path is set
to something that's not valid, and it might also be OK to not warn.
Right now we emit a NOTICE and I don't feel terribly upset about that;
even if I did, I don't know that it's really worth breaking backward
compatibility for.

The WARNING on login is more troubling to me, because it's
misdirected.  The warning is the result either of a setting that was
never valid in the first place, or of a setting that became invalid
when a schema was renamed or dropped.  The people responsible for the
breakage are not necessarily the same people being warned; the people
being warned may not even have power to fix the problem.

I think that part of the issue here is that it feels to you, as a
developer, that the per-user and per-database settings are applied on
top of the default from postgresql.conf.  But the user doesn't think
of it that way, I think.  To them, they expect the per-user or
per-database setting to "be there from the beginning", even though
that might not really be possible from an implementation perspective.
So they don't think of it being "applied" at startup time, and the
warning seems like a random intrusion (aside from possibly being log
spam).

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Apr 4, 2012 at 12:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> You're getting squishy on me ...

> My feeling on this is that it's OK to warn if the search_path is set
> to something that's not valid, and it might also be OK to not warn.
> Right now we emit a NOTICE and I don't feel terribly upset about that;
> even if I did, I don't know that it's really worth breaking backward
> compatibility for.

> The WARNING on login is more troubling to me, because it's
> misdirected.  The warning is the result either of a setting that was
> never valid in the first place, or of a setting that became invalid
> when a schema was renamed or dropped.  The people responsible for the
> breakage are not necessarily the same people being warned; the people
> being warned may not even have power to fix the problem.

Well, we don't have any ability to nag "the people responsible",
assuming that those really are different people.  The real question to
me is whether we should produce no warning whatsoever despite the fact
that the setting is failing to operate as intended.  That's not
particularly cool either IMO.  I answered a question in pgsql-novice
just a couple hours ago that I think demonstrates very well the problems
with failing to issue any message about something not doing what it
could be expected to:
http://archives.postgresql.org/pgsql-novice/2012-04/msg00008.php

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.
But as soon as you say "I want warnings in some cases", I think we have
a mess that nobody is ever going to be happy with, because there will
never be a clear and correct definition of which cases should get
warnings.

In any case, I think we might be converging on an agreement that the
setting should be *applied* if syntactically correct, whether or not
we are agreed about producing a NOTICE or WARNING for unknown schemas.
If I have not lost track, that is what happened before 9.1 but is not
what is happening now, and we should change it to fix that compatibility
break, independently of the question about messaging.
        regards, tom lane

On Wed, Apr 4, 2012 at 12:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.
> But as soon as you say "I want warnings in some cases", I think we have
> a mess that nobody is ever going to be happy with, because there will
> never be a clear and correct definition of which cases should get
> warnings.

I'm not sure I'm ready to sign on the dotted line with respect to
every aspect of your argument here, but that definition seems OK to
me.  In practice it's likely that a lot of the NOTICEs we emit now
will only be seen when restoring dumps, and certainly in that case
it's just junk anyway.  So I think this would be fine.

> In any case, I think we might be converging on an agreement that the
> setting should be *applied* if syntactically correct, whether or not
> we are agreed about producing a NOTICE or WARNING for unknown schemas.
> If I have not lost track, that is what happened before 9.1 but is not
> what is happening now, and we should change it to fix that compatibility
> break, independently of the question about messaging.

Sounds that way.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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.

It would be logical to treat all other cases the same. I then could
put the search_path into my .psqlrc and then have a "one size fits
all" search path for all my databases, etc...

> But as soon as you say "I want warnings in some cases", I think we have
> a mess that nobody is ever going to be happy with, because there will
> never be a clear and correct definition of which cases should get
> warnings.

As it looks impossible to divide the gray area, I'd opt to just drop
the warning and accept all syntactically valid strings.

Christoph
--
cb@df7cb.de | http://www.df7cb.de/

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');

On Tue, Apr 10, 2012 at 5:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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?

I guess my feeling would be "no", because it seems like a clear
behavior change, even though I agree the new behavior's better.  Since
my original investigation was prompted by a customer complaint, it's
tempting to say we should, but there's not much good making customer A
happy if we make customer B unhappy with the same change.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 10, 2012 at 5:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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?

> I guess my feeling would be "no", because it seems like a clear
> behavior change, even though I agree the new behavior's better.  Since
> my original investigation was prompted by a customer complaint, it's
> tempting to say we should, but there's not much good making customer A
> happy if we make customer B unhappy with the same change.

Well, although it's a behavior change, it consists entirely of removing
an error check.  To suppose that this would break somebody's app,
you'd have to suppose that they were relying on "SET search_path =
no_such_schema" to throw an error.  That's possible I guess, but it
seems significantly less likely than that somebody would be expecting
the ALTER ... SET case to not result in warnings.  There are
considerably cheaper and easier-to-use methods for checking whether a
schema exists than catching an error.

Anyway, if you're happy with 9.1 being an outlier on this behavior,
I won't press the point.
        regards, tom lane

On Tue, Apr 10, 2012 at 7:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Apr 10, 2012 at 5:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> 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?
>
>> I guess my feeling would be "no", because it seems like a clear
>> behavior change, even though I agree the new behavior's better.  Since
>> my original investigation was prompted by a customer complaint, it's
>> tempting to say we should, but there's not much good making customer A
>> happy if we make customer B unhappy with the same change.
>
> Well, although it's a behavior change, it consists entirely of removing
> an error check.  To suppose that this would break somebody's app,
> you'd have to suppose that they were relying on "SET search_path =
> no_such_schema" to throw an error.  That's possible I guess, but it
> seems significantly less likely than that somebody would be expecting
> the ALTER ... SET case to not result in warnings.  There are
> considerably cheaper and easier-to-use methods for checking whether a
> schema exists than catching an error.
>
> Anyway, if you're happy with 9.1 being an outlier on this behavior,
> I won't press the point.

I'm not, particularly.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 10, 2012 at 7:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Anyway, if you're happy with 9.1 being an outlier on this behavior,
>> I won't press the point.

> I'm not, particularly.

Well, the other thing we could do is tweak the rules for when to print a
complaint.  I notice that in check_temp_tablespaces we use the rule
source == PGC_S_SESSION (ie, SET) -> errorsource == PGC_S_TEST (testing value for ALTER SET) -> noticeelse -> silently
ignorebad name
 

which seems like it could be applied to search_path without giving
anyone grounds for complaint.  I'm still in favor of the previous patch
for HEAD, but maybe we could do this in 9.1.
        regards, tom lane

On Tue, Apr 10, 2012 at 9:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Apr 10, 2012 at 7:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Anyway, if you're happy with 9.1 being an outlier on this behavior,
>>> I won't press the point.
>
>> I'm not, particularly.
>
> Well, the other thing we could do is tweak the rules for when to print a
> complaint.  I notice that in check_temp_tablespaces we use the rule
>
>        source == PGC_S_SESSION (ie, SET) -> error
>        source == PGC_S_TEST (testing value for ALTER SET) -> notice
>        else -> silently ignore bad name
>
> which seems like it could be applied to search_path without giving
> anyone grounds for complaint.  I'm still in favor of the previous patch
> for HEAD, but maybe we could do this in 9.1.

Would that amount to removing the WARNING that was added in 9.1?  If
so, I think I could sign on to that proposal.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 10, 2012 at 9:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Well, the other thing we could do is tweak the rules for when to print a
>> complaint.  I notice that in check_temp_tablespaces we use the rule
>> 
>>        source == PGC_S_SESSION (ie, SET) -> error
>>        source == PGC_S_TEST (testing value for ALTER SET) -> notice
>>        else -> silently ignore bad name
>> 
>> which seems like it could be applied to search_path without giving
>> anyone grounds for complaint.  I'm still in favor of the previous patch
>> for HEAD, but maybe we could do this in 9.1.

> Would that amount to removing the WARNING that was added in 9.1?  If
> so, I think I could sign on to that proposal.

It would remove the warning that occurs while applying ALTER ... SET
values.  Another case that would change behavior is PGC_S_CLIENT;
I observe that 9.1 rejects bad settings there entirely:
$ PGOPTIONS="--search_path=foo" psqlpsql: FATAL:  invalid value for parameter "search_path": "foo"DETAIL:  schema "foo"
doesnot exist
 

but this did not happen in 9.0 so that seems like an improvement too.
I believe that the other possible source values all correspond to cases
where check_search_path would be executed outside a transaction and so
would not do the check in question anyway.  I've not tried to prove
that exhaustively though.
        regards, tom lane