setting per-database/role parameters checks them against wrong context

Started by Peter Eisentrautover 13 years ago11 messages

peter_e@gmx.net

over 13 years ago

Example:

create temporary table foo (a int);
insert into foo values (1);
alter role peter set temp_buffers = 120;
ERROR: 22023: invalid value for parameter "temp_buffers": 120
DETAIL: "temp_buffers" cannot be changed after any temporary tables
have been accessed in the session.

Another example:

set log_statement_stats = on;
alter role peter set log_parser_stats = on;
ERROR: 22023: invalid value for parameter "log_parser_stats": 1
DETAIL: Cannot enable parameter when "log_statement_stats" is true.

Another example is that in <=9.1, ALTER DATABASE ... SET search_path
would check the existence of the schema in the current database, but
that was done away with in 9.2.

The first example could probably be fixed if check_temp_buffers() paid
attention to the GucSource, but in the second case and in general there
doesn't seem to be a way to distinguish "assigning per-database setting"
and "enacting per-database setting" as a source.

Ideas?

Tom Lane

tgl@sss.pgh.pa.us

over 13 years ago

In reply to: Peter Eisentraut (#1)

Re: setting per-database/role parameters checks them against wrong context

Peter Eisentraut <peter_e@gmx.net> writes:

Example:
create temporary table foo (a int);
insert into foo values (1);
alter role peter set temp_buffers = 120;
ERROR: 22023: invalid value for parameter "temp_buffers": 120
DETAIL: "temp_buffers" cannot be changed after any temporary tables
have been accessed in the session.

Another example:

set log_statement_stats = on;
alter role peter set log_parser_stats = on;
ERROR: 22023: invalid value for parameter "log_parser_stats": 1
DETAIL: Cannot enable parameter when "log_statement_stats" is true.

Another example is that in <=9.1, ALTER DATABASE ... SET search_path
would check the existence of the schema in the current database, but
that was done away with in 9.2.

The first example could probably be fixed if check_temp_buffers() paid
attention to the GucSource, but in the second case and in general there
doesn't seem to be a way to distinguish "assigning per-database setting"
and "enacting per-database setting" as a source.

Ideas?

Perhaps instead of trying to solve the problem as stated, it would be
more useful to put the effort into getting rid of context-sensitive
restrictions on GUC settings. Neither of the examples above seem
particularly essential - they are just protecting incomplete
implementations.

regards, tom lane

Selena Deckelmann

selena@chesnok.com

over 13 years ago

In reply to: Tom Lane (#2)

Re: setting per-database/role parameters checks them against wrong context

On Fri, Sep 28, 2012 at 7:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

Example:
create temporary table foo (a int);
insert into foo values (1);
alter role peter set temp_buffers = 120;
ERROR: 22023: invalid value for parameter "temp_buffers": 120
DETAIL: "temp_buffers" cannot be changed after any temporary tables
have been accessed in the session.

Another example:

set log_statement_stats = on;
alter role peter set log_parser_stats = on;
ERROR: 22023: invalid value for parameter "log_parser_stats": 1
DETAIL: Cannot enable parameter when "log_statement_stats" is true.

Another example is that in <=9.1, ALTER DATABASE ... SET search_path
would check the existence of the schema in the current database, but
that was done away with in 9.2.

The first example could probably be fixed if check_temp_buffers() paid
attention to the GucSource, but in the second case and in general there
doesn't seem to be a way to distinguish "assigning per-database setting"
and "enacting per-database setting" as a source.

Ideas?

Perhaps instead of trying to solve the problem as stated, it would be
more useful to put the effort into getting rid of context-sensitive
restrictions on GUC settings. Neither of the examples above seem
particularly essential - they are just protecting incomplete
implementations.

The check_temp_buffers() problem seems like a regression and blocks us
from upgrading to 9.2. The use case are functions that set
temp_buffers and occasionally are called in a series from a parent
session. The work around is... a lot of work.

I'd appreciate it if we could fix the temp_buffers issue, and I
support getting rid of context-sensitive restrictions. :)

-selena

--
http://chesnok.com

Tom Lane

tgl@sss.pgh.pa.us

over 13 years ago

In reply to: Selena Deckelmann (#3)

Re: setting per-database/role parameters checks them against wrong context

Selena Deckelmann <selena@chesnok.com> writes:

The check_temp_buffers() problem seems like a regression and blocks us
from upgrading to 9.2. The use case are functions that set
temp_buffers and occasionally are called in a series from a parent
session. The work around is... a lot of work.

Uh ... how is that a regression? AFAIK it's been that way right along.

regards, tom lane

Josh Berkus

josh@agliodbs.com

over 13 years ago

In reply to: Tom Lane (#4)

Re: setting per-database/role parameters checks them against wrong context

Uh ... how is that a regression? AFAIK it's been that way right along.

No, it hasn't. I currently have an application whose functions, each of
which sets temp_buffers, works fine under 9.0 and ERRORs out under 9.2.
It's blocking an upgrade.

This is new.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Selena Deckelmann

selena@chesnok.com

over 13 years ago

In reply to: Tom Lane (#4)

Re: setting per-database/role parameters checks them against wrong context

On Mon, Oct 1, 2012 at 1:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Selena Deckelmann <selena@chesnok.com> writes:

The check_temp_buffers() problem seems like a regression and blocks us
from upgrading to 9.2. The use case are functions that set
temp_buffers and occasionally are called in a series from a parent
session. The work around is... a lot of work.

Uh ... how is that a regression? AFAIK it's been that way right along.

We're running 9.0 - looks like it changed in 9.1, last revision to the
relevant line was 6/2011. The group decided not to upgrade to 9.1 last
year, but was going to just go directly to 9.2 in the next few weeks.

-selena

--
http://chesnok.com

Selena Deckelmann

selena@chesnok.com

over 13 years ago

In reply to: Selena Deckelmann (#6)

Re: setting per-database/role parameters checks them against wrong context

On Mon, Oct 1, 2012 at 1:37 PM, Selena Deckelmann <selena@chesnok.com> wrote:

On Mon, Oct 1, 2012 at 1:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Selena Deckelmann <selena@chesnok.com> writes:

The check_temp_buffers() problem seems like a regression and blocks us
from upgrading to 9.2. The use case are functions that set
temp_buffers and occasionally are called in a series from a parent
session. The work around is... a lot of work.

Uh ... how is that a regression? AFAIK it's been that way right along.

We're running 9.0 - looks like it changed in 9.1, last revision to the
relevant line was 6/2011. The group decided not to upgrade to 9.1 last
year, but was going to just go directly to 9.2 in the next few weeks.

And, I was basing the regression comment on the documentation for
temp_buffers: "The setting can be changed within individual sessions,
but only before the first use of temporary tables within the session;
subsequent attempts to change the value will have no effect on that
session." This statement has been there since at least 8.1.

A warning, and then not failing seems more appropriate than an error,
given the documented behavior.

-selena

--
http://chesnok.com

Selena Deckelmann

selena@chesnok.com

over 13 years ago

In reply to: Selena Deckelmann (#7)

1 attachment(s)

Re: setting per-database/role parameters checks them against wrong context

On Mon, Oct 1, 2012 at 2:28 PM, Selena Deckelmann <selena@chesnok.com> wrote:

On Mon, Oct 1, 2012 at 1:37 PM, Selena Deckelmann <selena@chesnok.com> wrote:

On Mon, Oct 1, 2012 at 1:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Selena Deckelmann <selena@chesnok.com> writes:

The check_temp_buffers() problem seems like a regression and blocks us
from upgrading to 9.2. The use case are functions that set
temp_buffers and occasionally are called in a series from a parent
session. The work around is... a lot of work.

Uh ... how is that a regression? AFAIK it's been that way right along.

We're running 9.0 - looks like it changed in 9.1, last revision to the
relevant line was 6/2011. The group decided not to upgrade to 9.1 last
year, but was going to just go directly to 9.2 in the next few weeks.

And, I was basing the regression comment on the documentation for
temp_buffers: "The setting can be changed within individual sessions,
but only before the first use of temporary tables within the session;
subsequent attempts to change the value will have no effect on that
session." This statement has been there since at least 8.1.

A warning, and then not failing seems more appropriate than an error,
given the documented behavior.

I tried out a few things, and then realized that perhaps just adding
PGC_S_SESSION to the list of sources that are at elevel WARNING
partially fixes this.

This doesn't fix the issue with log_statement_stats, but it makes the
behavior of temp_buffers the documented behavior (no longer errors
out in a transaction), while still warning the user.

-selena

--
http://chesnok.com

Attachments:

session_warning.patchapplication/octet-stream; name=session_warning.patchDownload

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 6b202e0..0677059 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5150,7 +5150,7 @@ set_config_option(const char *name, const char *value,
 			elevel = IsUnderPostmaster ? DEBUG3 : LOG;
 		}
 		else if (source == PGC_S_DATABASE || source == PGC_S_USER ||
-				 source == PGC_S_DATABASE_USER)
+				 source == PGC_S_DATABASE_USER || source == PG_S_SESSION)
 			elevel = WARNING;
 		else
 			elevel = ERROR;

Bruce Momjian

bruce@momjian.us

almost 13 years ago

In reply to: Selena Deckelmann (#8)

Re: setting per-database/role parameters checks them against wrong context

On Sat, Oct 6, 2012 at 02:20:53PM -0700, Selena Deckelmann wrote:

On Mon, Oct 1, 2012 at 2:28 PM, Selena Deckelmann <selena@chesnok.com> wrote:

On Mon, Oct 1, 2012 at 1:37 PM, Selena Deckelmann <selena@chesnok.com> wrote:

On Mon, Oct 1, 2012 at 1:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Selena Deckelmann <selena@chesnok.com> writes:

The check_temp_buffers() problem seems like a regression and blocks us
from upgrading to 9.2. The use case are functions that set
temp_buffers and occasionally are called in a series from a parent
session. The work around is... a lot of work.

Uh ... how is that a regression? AFAIK it's been that way right along.

We're running 9.0 - looks like it changed in 9.1, last revision to the
relevant line was 6/2011. The group decided not to upgrade to 9.1 last
year, but was going to just go directly to 9.2 in the next few weeks.

And, I was basing the regression comment on the documentation for
temp_buffers: "The setting can be changed within individual sessions,
but only before the first use of temporary tables within the session;
subsequent attempts to change the value will have no effect on that
session." This statement has been there since at least 8.1.

A warning, and then not failing seems more appropriate than an error,
given the documented behavior.

I tried out a few things, and then realized that perhaps just adding
PGC_S_SESSION to the list of sources that are at elevel WARNING
partially fixes this.

This doesn't fix the issue with log_statement_stats, but it makes the
behavior of temp_buffers the documented behavior (no longer errors
out in a transaction), while still warning the user.

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 6b202e0..0677059 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5150,7 +5150,7 @@ set_config_option(const char *name, const char *value,
elevel = IsUnderPostmaster ? DEBUG3 : LOG;
}
else if (source == PGC_S_DATABASE || source == PGC_S_USER ||
-				 source == PGC_S_DATABASE_USER)
+				 source == PGC_S_DATABASE_USER || source == PG_S_SESSION)
elevel = WARNING;
else
elevel = ERROR;

Is there any opinion on whether we need this patch? It basically allows
SET from a session to issue a warning rather than an error. This is
certainly a much larger change than just fixing temp_buffers. The user
complaint was that functions were setting temp_buffers and getting
errors because temp table has already been used:

The setting can be changed within individual sessions, but only
before the first use of temporary tables within the session;
subsequent attempts to change the value will have no effect on
that session.

The full thread is here:

/messages/by-id/1348802984.3584.22.camel@vanquo.pezone.net

Seems this changed in PG 9.1.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10

Tom Lane

tgl@sss.pgh.pa.us

almost 13 years ago

In reply to: Bruce Momjian (#9)

Re: setting per-database/role parameters checks them against wrong context

Bruce Momjian <bruce@momjian.us> writes:

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 6b202e0..0677059 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5150,7 +5150,7 @@ set_config_option(const char *name, const char *value,
elevel = IsUnderPostmaster ? DEBUG3 : LOG;
}
else if (source == PGC_S_DATABASE || source == PGC_S_USER ||
-				 source == PGC_S_DATABASE_USER)
+				 source == PGC_S_DATABASE_USER || source == PG_S_SESSION)
elevel = WARNING;
else
elevel = ERROR;

Is there any opinion on whether we need this patch? It basically allows
SET from a session to issue a warning rather than an error.

Surely this is a completely horrid idea. It doesn't "allow" SET to
throw a warning, it changes all interactive-SET cases from ERROR to
WARNING. That's a whole lot of collateral damage to fix a very narrow
case that's not even there anymore.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11

Bruce Momjian

bruce@momjian.us

almost 13 years ago

In reply to: Tom Lane (#10)

Re: setting per-database/role parameters checks them against wrong context

On Fri, Jan 25, 2013 at 03:35:59PM -0500, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 6b202e0..0677059 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5150,7 +5150,7 @@ set_config_option(const char *name, const char *value,
elevel = IsUnderPostmaster ? DEBUG3 : LOG;
}
else if (source == PGC_S_DATABASE || source == PGC_S_USER ||
-				 source == PGC_S_DATABASE_USER)
+				 source == PGC_S_DATABASE_USER || source == PG_S_SESSION)
elevel = WARNING;
else
elevel = ERROR;
Is there any opinion on whether we need this patch? It basically allows
SET from a session to issue a warning rather than an error.

Surely this is a completely horrid idea. It doesn't "allow" SET to
throw a warning, it changes all interactive-SET cases from ERROR to
WARNING. That's a whole lot of collateral damage to fix a very narrow
case that's not even there anymore.

Agreed.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers