invalid search_path complaints

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

On Tue, Apr 3, 2012 at 2:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

I have a legacy application now that relies heavily on multiple databases

and multiple schemas. The issue I have is that we have postgres deployed
very widely and have a cookie-cutter script for everything. We know for
example:

(each schema exists only in its respective DB)

user oltp should be able to see schemaA in db1, schemaB in db2 and
schemaC in db3
user reporting should be able to see biSchema in db1, reporting schema
in db2 and schemaC in db3

This is across a large multitude of databases and hosts. One of the
things I've loved about this is the ability to hide certain things from
users ( of course they can do a \dn and fully-qualify, which is why we have
permissions too, but I really appreciate the 'hidden-ness' of my tables ).

Because our schemas are all over the place, now I've got to setup a
hard-coded search_path in postgresql.conf which feels even worse to me than
the per-user setup.

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 :)

Just my two cents.

--Scott
OpenSCG
http://www.openscg.com

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

On Wed, Apr 4, 2012 at 12:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

I think we should always accept a syntactically valid search_path.

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.

--Scott

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

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) -> 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.

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" psql
psql: FATAL: invalid value for parameter "search_path": "foo"
DETAIL: schema "foo" does not 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