Thread: setting per-database/role parameters checks them against wrong context
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?
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
Re: setting per-database/role parameters checks them against wrong context
From
Selena Deckelmann
Date:
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
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
> 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
Re: setting per-database/role parameters checks them against wrong context
From
Selena Deckelmann
Date:
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
Re: setting per-database/role parameters checks them against wrong context
From
Selena Deckelmann
Date:
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
Re: setting per-database/role parameters checks them against wrong context
From
Selena Deckelmann
Date:
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
Attachment
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 withinthe session; subsequent attempts to change the value will have no effect on that session. The full thread is here: http://www.postgresql.org/message-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. +
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
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. +