Thread: [HACKERS] PoC plpgsql - possibility to force custom or generic plan
Hi,
this patch is based on discussions related to plpgsql2 project.
Currently we cannot to control plan cache from plpgsql directly. We can use dynamic SQL if we can enforce oneshot plan - but it means little bit less readable code (if we enforce dynamic SQL from performance reasons). It means so the code cannot be checked by plpgsql check too.
The plan cache subsystem allows some control by options CURSOR_OPT_GENERIC_PLAN and CURSOR_OPT_CUSTOM_PLAN. So we just a interface how to use these options from PLpgSQL. I used Ada language feature (used in PL/SQL too) - PRAGMA statement. It allows to set compiler directives. The syntax of PRAGMA statements allows to set a level where entered compiler directive should be applied. It can works on function level or block level.
Attached patch introduces PRAGMA plan_cache with options: DEFAULT, FORCE_CUSTOM_PLAN, FORCE_GENERIC_PLAN. Plan cache is partially used every time - the parser/analyzer result is cached every time.
Examples:
CREATE OR REPLACE FUNCTION foo(a int)
RETURNS int AS $$
DECLARE ..
BEGIN
DECLARE
/* block level (local scope) pragma */
PRAGMA plan_cache(FORCE_CUSTOM_PLAN);
BEGIN
SELECT /* slow query - dynamic sql is not necessary */
END;
END;
Benefits:
1. remove one case where dynamic sql is necessary now - security, static check
2. introduce PRAGMAs - possible usage: autonomous transactions, implicit namespaces settings (namespace for auto variables, namespace for function arguments).
Comments, notes?
Regards
Pavel
Attachment
On 1/23/17 2:10 PM, Pavel Stehule wrote: > Comments, notes? +1 on the idea. It'd also be nice if we could expose control of plans for dynamic SQL, though I suspect that's not terribly useful without some kind of global session storage. A couple notes on a quick read-through: Instead of paralleling all the existing namespace stuff, I wonder if it'd be better to create explicit block infrastructure. AFAIK PRAGMAs are going to have a lot of the same requirements (certainly the nesting is the same), and we might want more of this king of stuff in the future. (I've certainly wished I could set a GUC in a plpgsql block and have it's settings revert when exiting the block...) Perhaps that's as simple as renaming all the existing _ns_* functions to _block_ and then adding support for pragmas... Since you're adding cursor_options to PLpgSQL_expr it should probably be removed as an option to exec_*. finit_ would be better named free_. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532)
Hi
2017-01-23 21:59 GMT+01:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
On 1/23/17 2:10 PM, Pavel Stehule wrote:Comments, notes?
+1 on the idea. It'd also be nice if we could expose control of plans for dynamic SQL, though I suspect that's not terribly useful without some kind of global session storage.
A couple notes on a quick read-through:
Instead of paralleling all the existing namespace stuff, I wonder if it'd be better to create explicit block infrastructure. AFAIK PRAGMAs are going to have a lot of the same requirements (certainly the nesting is the same), and we might want more of this king of stuff in the future. (I've certainly wished I could set a GUC in a plpgsql block and have it's settings revert when exiting the block...)
I am not sure if I understand. ?? Setting GUC by PRAGMA can work - the syntax supports it and GUC API supports nesting. Not sure about exception handling - but it should not be problem probably.
Please, can you show some examples.
Perhaps that's as simple as renaming all the existing _ns_* functions to _block_ and then adding support for pragmas...
Since you're adding cursor_options to PLpgSQL_expr it should probably be removed as an option to exec_*.
I have to recheck it. Some cursor options going from dynamic cursor variables and are related to dynamic query - not query that creates query string.
finit_ would be better named free_.
good idea
Regards
Pavel
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
2017-01-23 21:59 GMT+01:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
On 1/23/17 2:10 PM, Pavel Stehule wrote:Comments, notes?
+1 on the idea. It'd also be nice if we could expose control of plans for dynamic SQL, though I suspect that's not terribly useful without some kind of global session storage.
yes - it requires classic normalised query string controlled plan cache.
But same plan cache options can be valid for prepared statements - probably with GUC. This is valid theme, but out of my proposal in this moment.
Regards
Pavel
2017-01-24 6:38 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com>:
Hi2017-01-23 21:59 GMT+01:00 Jim Nasby <Jim.Nasby@bluetreble.com>:On 1/23/17 2:10 PM, Pavel Stehule wrote:Comments, notes?
+1 on the idea. It'd also be nice if we could expose control of plans for dynamic SQL, though I suspect that's not terribly useful without some kind of global session storage.
A couple notes on a quick read-through:
Instead of paralleling all the existing namespace stuff, I wonder if it'd be better to create explicit block infrastructure. AFAIK PRAGMAs are going to have a lot of the same requirements (certainly the nesting is the same), and we might want more of this king of stuff in the future. (I've certainly wished I could set a GUC in a plpgsql block and have it's settings revert when exiting the block...)I am not sure if I understand. ?? Setting GUC by PRAGMA can work - the syntax supports it and GUC API supports nesting. Not sure about exception handling - but it should not be problem probably.Please, can you show some examples.
Perhaps that's as simple as renaming all the existing _ns_* functions to _block_ and then adding support for pragmas...
Since you're adding cursor_options to PLpgSQL_expr it should probably be removed as an option to exec_*.I have to recheck it. Some cursor options going from dynamic cursor variables and are related to dynamic query - not query that creates query string.
hmm .. so current state is better due using options like CURSOR_OPT_PARALLEL_OK
if (expr->plan == NULL)
exec_prepare_plan(estate, expr, (parallelOK ?
CURSOR_OPT_PARALLEL_OK : 0) | expr->cursor_options);
if (expr->plan == NULL)
exec_prepare_plan(estate, expr, (parallelOK ?
CURSOR_OPT_PARALLEL_OK : 0) | expr->cursor_options);
This options is not permanent feature of expression - and then I cannot to remove cursor_option argument from exec_*
I did minor cleaning - remove cursor_options from plpgsql_var
Regards
Pavel
finit_ would be better named free_.good ideaRegardsPavel
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
Attachment
On 1/23/17 11:38 PM, Pavel Stehule wrote: > > Instead of paralleling all the existing namespace stuff, I wonder if > it'd be better to create explicit block infrastructure. AFAIK > PRAGMAs are going to have a lot of the same requirements (certainly > the nesting is the same), and we might want more of this king of > stuff in the future. (I've certainly wished I could set a GUC in a > plpgsql block and have it's settings revert when exiting the block...) > > > I am not sure if I understand. ?? Setting GUC by PRAGMA can work - the > syntax supports it and GUC API supports nesting. Not sure about > exception handling - but it should not be problem probably. > > Please, can you show some examples. From a code standpoint, there's already some ugliness around blocks: there's the code that handles blocks themselves (which IIRC is responsible for subtransactions), then there's the namespace code, which is very separate even though namespaces are very much tied to blocks. Your patch is adding another layer into the mix, separate from both blocks and namespaces. I think it would be better to combine all 3 together, or at least not make matters worse. So IMHO the pragma stuff should be part of handling blocks, and not something that's stand alone. IE: make the pragma info live in PLpgSQL_stmt_block. GUCs support SET LOCAL, but that's not the same as local scoping because the setting stays in effect unless the substrans aborts. What I'd like is the ability to set a GUC in a plpgsql block *and have the setting revert on block exit*. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532)
2017-01-25 21:06 GMT+01:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
On 1/23/17 11:38 PM, Pavel Stehule wrote:
Instead of paralleling all the existing namespace stuff, I wonder if
it'd be better to create explicit block infrastructure. AFAIK
PRAGMAs are going to have a lot of the same requirements (certainly
the nesting is the same), and we might want more of this king of
stuff in the future. (I've certainly wished I could set a GUC in a
plpgsql block and have it's settings revert when exiting the block...)
I am not sure if I understand. ?? Setting GUC by PRAGMA can work - the
syntax supports it and GUC API supports nesting. Not sure about
exception handling - but it should not be problem probably.
Please, can you show some examples.
From a code standpoint, there's already some ugliness around blocks: there's the code that handles blocks themselves (which IIRC is responsible for subtransactions), then there's the namespace code, which is very separate even though namespaces are very much tied to blocks. Your patch is adding another layer into the mix, separate from both blocks and namespaces. I think it would be better to combine all 3 together, or at least not make matters worse. So IMHO the pragma stuff should be part of handling blocks, and not something that's stand alone. IE: make the pragma info live in PLpgSQL_stmt_block.
I don't think it is fully correct - the pragma can be related to function too - and namespaces can be related to some other statements - cycles. Any PLpgSQL_stmt_block does some overhead and probably we want to build a fake statements to ensure 1:1 relations between namespaces and blocks.
I didn't implement and proposed third level of pragma - statement. For example the assertions in Ada language are implemented with pragma. Currently I am not thinking about this form for Postgres.
The cursor options is better stored in expression - the block related GUC probably should be stored in stmt_block. The pragma is additional information, and how this information will be used and what impact will be on generated code depends on pragma - can be different.
GUCs support SET LOCAL, but that's not the same as local scoping because the setting stays in effect unless the substrans aborts. What I'd like is the ability to set a GUC in a plpgsql block *and have the setting revert on block exit*.
I am think so it is solvable.
Regards
Pavel
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
On 25 January 2017 at 20:06, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: > GUCs support SET LOCAL, but that's not the same as local scoping because the > setting stays in effect unless the substrans aborts. What I'd like is the > ability to set a GUC in a plpgsql block *and have the setting revert on > block exit*. I'm wondering which GUCs you have in mind to use this with. Because what you're describing is dynamic scoping and I'm wondering if what you're really looking for is lexical scoping. That would be more in line with how PL/PgSQL variables are scoped and with how #pragmas usually work. But it would probably not be easy to reconcile with how GUCs work. -- greg
Hi
2017-01-24 21:33 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com>:
Perhaps that's as simple as renaming all the existing _ns_* functions to _block_ and then adding support for pragmas...
Since you're adding cursor_options to PLpgSQL_expr it should probably be removed as an option to exec_*.I have to recheck it. Some cursor options going from dynamic cursor variables and are related to dynamic query - not query that creates query string.hmm .. so current state is better due using options like CURSOR_OPT_PARALLEL_OK
if (expr->plan == NULL)
exec_prepare_plan(estate, expr, (parallelOK ?
CURSOR_OPT_PARALLEL_OK : 0) | expr->cursor_options);This options is not permanent feature of expression - and then I cannot to remove cursor_option argument from exec_*I did minor cleaning - remove cursor_options from plpgsql_varRegardsPavel
+ basic doc
Regards
Pavel
Attachment
On 1/27/17 4:14 AM, Greg Stark wrote: > On 25 January 2017 at 20:06, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: >> GUCs support SET LOCAL, but that's not the same as local scoping because the >> setting stays in effect unless the substrans aborts. What I'd like is the >> ability to set a GUC in a plpgsql block *and have the setting revert on >> block exit*. > > I'm wondering which GUCs you have in mind to use this with. It's been quite some time since I messed with this; the only case I remember right now is wanting to do a temporary SET ROLE in an "exec" function: SELECT tools.exec( 'some sql;', role := 'superuser_role' ); To do that, exec has to remember what the current role is and then set it back (as well as remembering to do SET LOCAL in case an error happens. > Because what you're describing is dynamic scoping and I'm wondering if > what you're really looking for is lexical scoping. That would be more > in line with how PL/PgSQL variables are scoped and with how #pragmas > usually work. But it would probably not be easy to reconcile with how > GUCs work. Right, because GUCs aren't even simply dynamically scoped; they're dynamically scoped with transaction support. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532)
On 2/1/17 3:59 PM, Pavel Stehule wrote: > Hi > > 2017-01-24 21:33 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com > <mailto:pavel.stehule@gmail.com>>: > > Perhaps that's as simple as renaming all the existing _ns_* > functions to _block_ and then adding support for pragmas... > > Since you're adding cursor_options to PLpgSQL_expr it should > probably be removed as an option to exec_*. > > I have to recheck it. Some cursor options going from dynamic > cursor variables and are related to dynamic query - not query > that creates query string. > > hmm .. so current state is better due using options like > CURSOR_OPT_PARALLEL_OK > > if (expr->plan == NULL) > exec_prepare_plan(estate, expr, (parallelOK ? > CURSOR_OPT_PARALLEL_OK : 0) | > expr->cursor_options); > > This options is not permanent feature of expression - and then I > cannot to remove cursor_option argument from exec_* > > I did minor cleaning - remove cursor_options from plpgsql_var > > + basic doc This patch still applies cleanly and compiles at cccbdde. Any reviewers want to have a look? -- -David david@pgmasters.net
On 16/03/17 17:15, David Steele wrote: > On 2/1/17 3:59 PM, Pavel Stehule wrote: >> Hi >> >> 2017-01-24 21:33 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com >> <mailto:pavel.stehule@gmail.com>>: >> >> Perhaps that's as simple as renaming all the existing _ns_* >> functions to _block_ and then adding support for pragmas... >> >> Since you're adding cursor_options to PLpgSQL_expr it should >> probably be removed as an option to exec_*. >> >> I have to recheck it. Some cursor options going from dynamic >> cursor variables and are related to dynamic query - not query >> that creates query string. >> >> hmm .. so current state is better due using options like >> CURSOR_OPT_PARALLEL_OK >> >> if (expr->plan == NULL) >> exec_prepare_plan(estate, expr, (parallelOK ? >> CURSOR_OPT_PARALLEL_OK : 0) | >> expr->cursor_options); >> >> This options is not permanent feature of expression - and then I >> cannot to remove cursor_option argument from exec_* >> >> I did minor cleaning - remove cursor_options from plpgsql_var >> >> + basic doc > > This patch still applies cleanly and compiles at cccbdde. > > Any reviewers want to have a look? > I'll bite. I agree with Jim that it's not very nice to add yet another block/ns-like layer. I don't see why pragma could not be added to either PLpgSQL_stmt_block (yes pragma can be for whole function but function body is represented by PLpgSQL_stmt_block as well so no issue there), or to namespace code. In namespace since they are used for other thing there would be bit of unnecessary propagation but it's 8bytes per namespace, does not seem all that much. My preference would be to add it to PLpgSQL_stmt_block (unless we plan to add posibility to add pragmas for other loops and other things) but I am not sure if current block is easily (and in a fast way) accessible from all places where it's needed. Maybe the needed info could be pushed to estate from PLpgSQL_stmt_block during the execution. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
2017-03-18 19:30 GMT+01:00 Petr Jelinek <petr.jelinek@2ndquadrant.com>:
On 16/03/17 17:15, David Steele wrote:
> On 2/1/17 3:59 PM, Pavel Stehule wrote:
>> Hi
>>
>> 2017-01-24 21:33 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com
>> <mailto:pavel.stehule@gmail.com>>:
>>
>> Perhaps that's as simple as renaming all the existing _ns_*
>> functions to _block_ and then adding support for pragmas...
>>
>> Since you're adding cursor_options to PLpgSQL_expr it should
>> probably be removed as an option to exec_*.
>>
>> I have to recheck it. Some cursor options going from dynamic
>> cursor variables and are related to dynamic query - not query
>> that creates query string.
>>
>> hmm .. so current state is better due using options like
>> CURSOR_OPT_PARALLEL_OK
>>
>> if (expr->plan == NULL)
>> exec_prepare_plan(estate, expr, (parallelOK ?
>> CURSOR_OPT_PARALLEL_OK : 0) |
>> expr->cursor_options);
>>
>> This options is not permanent feature of expression - and then I
>> cannot to remove cursor_option argument from exec_*
>>
>> I did minor cleaning - remove cursor_options from plpgsql_var
>>
>> + basic doc
>
> This patch still applies cleanly and compiles at cccbdde.
>
> Any reviewers want to have a look?
>
I'll bite.
I agree with Jim that it's not very nice to add yet another
block/ns-like layer. I don't see why pragma could not be added to either
PLpgSQL_stmt_block (yes pragma can be for whole function but function
body is represented by PLpgSQL_stmt_block as well so no issue there), or
to namespace code. In namespace since they are used for other thing
there would be bit of unnecessary propagation but it's 8bytes per
namespace, does not seem all that much.
My preference would be to add it to PLpgSQL_stmt_block (unless we plan
to add posibility to add pragmas for other loops and other things) but I
am not sure if current block is easily (and in a fast way) accessible
from all places where it's needed. Maybe the needed info could be pushed
to estate from PLpgSQL_stmt_block during the execution.
There is maybe partial misunderstand of pragma - it is set of nested configurations used in compile time only. It can be used in execution time too - it change nothing.
The pragma doesn't build a persistent tree. It is stack of configurations that allows fast access to current configuration, and fast leaving of configuration when the change is out of scope.
I don't see any any advantage to integrate pragma to ns or to stmt_block. But maybe I don't understand to your idea.
I see a another possibility in code - nesting init_block_directives() to plpgsql_ns_push and free_block_directives() to plpgsql_ns_pop()
Pavel
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 19/03/17 12:32, Pavel Stehule wrote: > > > 2017-03-18 19:30 GMT+01:00 Petr Jelinek <petr.jelinek@2ndquadrant.com > <mailto:petr.jelinek@2ndquadrant.com>>: > > On 16/03/17 17:15, David Steele wrote: > > On 2/1/17 3:59 PM, Pavel Stehule wrote: > >> Hi > >> > >> 2017-01-24 21:33 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com <mailto:pavel.stehule@gmail.com> > >> <mailto:pavel.stehule@gmail.com <mailto:pavel.stehule@gmail.com>>>: > >> > >> Perhaps that's as simple as renaming all the existing _ns_* > >> functions to _block_ and then adding support for pragmas... > >> > >> Since you're adding cursor_options to PLpgSQL_expr it should > >> probably be removed as an option to exec_*. > >> > >> I have to recheck it. Some cursor options going from dynamic > >> cursor variables and are related to dynamic query - not query > >> that creates query string. > >> > >> hmm .. so current state is better due using options like > >> CURSOR_OPT_PARALLEL_OK > >> > >> if (expr->plan == NULL) > >> exec_prepare_plan(estate, expr, (parallelOK ? > >> CURSOR_OPT_PARALLEL_OK : 0) | > >> expr->cursor_options); > >> > >> This options is not permanent feature of expression - and then I > >> cannot to remove cursor_option argument from exec_* > >> > >> I did minor cleaning - remove cursor_options from plpgsql_var > >> > >> + basic doc > > > > This patch still applies cleanly and compiles at cccbdde. > > > > Any reviewers want to have a look? > > > > I'll bite. > > I agree with Jim that it's not very nice to add yet another > block/ns-like layer. I don't see why pragma could not be added to either > PLpgSQL_stmt_block (yes pragma can be for whole function but function > body is represented by PLpgSQL_stmt_block as well so no issue there), or > to namespace code. In namespace since they are used for other thing > there would be bit of unnecessary propagation but it's 8bytes per > namespace, does not seem all that much. > > My preference would be to add it to PLpgSQL_stmt_block (unless we plan > to add posibility to add pragmas for other loops and other things) but I > am not sure if current block is easily (and in a fast way) accessible > from all places where it's needed. Maybe the needed info could be pushed > to estate from PLpgSQL_stmt_block during the execution. > > > There is maybe partial misunderstand of pragma - it is set of nested > configurations used in compile time only. It can be used in execution > time too - it change nothing. > > The pragma doesn't build a persistent tree. It is stack of > configurations that allows fast access to current configuration, and > fast leaving of configuration when the change is out of scope. > > I don't see any any advantage to integrate pragma to ns or to > stmt_block. But maybe I don't understand to your idea. > > I see a another possibility in code - nesting init_block_directives() to > plpgsql_ns_push and free_block_directives() to plpgsql_ns_pop() > That's more or less what I mean by "integrating" to ns :) The main idea is to not add 3rd layer of block push/pop that's sprinkled in "random" places. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
2017-03-19 14:30 GMT+01:00 Petr Jelinek <petr.jelinek@2ndquadrant.com>:
On 19/03/17 12:32, Pavel Stehule wrote:
>
>
> 2017-03-18 19:30 GMT+01:00 Petr Jelinek <petr.jelinek@2ndquadrant.com
> <mailto:petr.jelinek@2ndquadrant.com>>:
>
> On 16/03/17 17:15, David Steele wrote:
> > On 2/1/17 3:59 PM, Pavel Stehule wrote:
> >> Hi
> >>
> >> 2017-01-24 21:33 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com <mailto:pavel.stehule@gmail.com>
> >> <mailto:pavel.stehule@gmail.com <mailto:pavel.stehule@gmail. com>>>: That's more or less what I mean by "integrating" to ns :)> >>
> >> Perhaps that's as simple as renaming all the existing _ns_*
> >> functions to _block_ and then adding support for pragmas...
> >>
> >> Since you're adding cursor_options to PLpgSQL_expr it should
> >> probably be removed as an option to exec_*.
> >>
> >> I have to recheck it. Some cursor options going from dynamic
> >> cursor variables and are related to dynamic query - not query
> >> that creates query string.
> >>
> >> hmm .. so current state is better due using options like
> >> CURSOR_OPT_PARALLEL_OK
> >>
> >> if (expr->plan == NULL)
> >> exec_prepare_plan(estate, expr, (parallelOK ?
> >> CURSOR_OPT_PARALLEL_OK : 0) |
> >> expr->cursor_options);
> >>
> >> This options is not permanent feature of expression - and then I
> >> cannot to remove cursor_option argument from exec_*
> >>
> >> I did minor cleaning - remove cursor_options from plpgsql_var
> >>
> >> + basic doc
> >
> > This patch still applies cleanly and compiles at cccbdde.
> >
> > Any reviewers want to have a look?
> >
>
> I'll bite.
>
> I agree with Jim that it's not very nice to add yet another
> block/ns-like layer. I don't see why pragma could not be added to either
> PLpgSQL_stmt_block (yes pragma can be for whole function but function
> body is represented by PLpgSQL_stmt_block as well so no issue there), or
> to namespace code. In namespace since they are used for other thing
> there would be bit of unnecessary propagation but it's 8bytes per
> namespace, does not seem all that much.
>
> My preference would be to add it to PLpgSQL_stmt_block (unless we plan
> to add posibility to add pragmas for other loops and other things) but I
> am not sure if current block is easily (and in a fast way) accessible
> from all places where it's needed. Maybe the needed info could be pushed
> to estate from PLpgSQL_stmt_block during the execution.
>
>
> There is maybe partial misunderstand of pragma - it is set of nested
> configurations used in compile time only. It can be used in execution
> time too - it change nothing.
>
> The pragma doesn't build a persistent tree. It is stack of
> configurations that allows fast access to current configuration, and
> fast leaving of configuration when the change is out of scope.
>
> I don't see any any advantage to integrate pragma to ns or to
> stmt_block. But maybe I don't understand to your idea.
>
> I see a another possibility in code - nesting init_block_directives() to
> plpgsql_ns_push and free_block_directives() to plpgsql_ns_pop()
>
The main idea is to not add 3rd layer of block push/pop that's sprinkled
in "random" places.
ok fixed
I reworked a maintaining settings - now it use lazy copy - the copy of settings is created only when pragma is used in namespace. It remove any impact on current code without pragmas.
Regards
Pavel
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Hi
rebased due last changes in pg_exec.c
Regards
Pavel
Attachment
On 28/03/17 19:43, Pavel Stehule wrote: > Hi > > rebased due last changes in pg_exec.c > Thanks, I went over this and worked over the documentation/comments a bit (attached updated version of the patch with my changes). From my side this can go to committer. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
2017-03-28 20:29 GMT+02:00 Petr Jelinek <petr.jelinek@2ndquadrant.com>:
On 28/03/17 19:43, Pavel Stehule wrote:
> Hi
>
> rebased due last changes in pg_exec.c
>
Thanks, I went over this and worked over the documentation/comments a
bit (attached updated version of the patch with my changes).
From my side this can go to committer.
Thank you
Pavel
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Hi,
I'd like some input from other committers whether we want this. I'm
somewhat doubtful, but don't have particularly strong feelings.
> +
> + <sect2 id="plpgsql-declaration-pragma">
> + <title>Block level PRAGMA</title>
> +
> + <indexterm>
> + <primary>PRAGMA</>
> + <secondary>in PL/pgSQL</>
> + </indexterm>
> +
> + <para>
> + The block level <literal>PRAGMA</literal> allows to change the
> + <application>PL/pgSQL</application> compiler behavior. Currently
> + only <literal>PRAGMA PLAN_CACHE</literal> is supported.
Why are we doing this on a block level?
> +<programlisting>
> +CREATE FUNCTION enforce_fresh_plan(_id text) RETURNS boolean AS $$
> +DECLARE
> + PRAGMA PLAN_CACHE(force_custom_plan);
> +BEGIN
> + -- in this block every embedded query uses one shot plan
*plans
> + <sect3 id="PRAGMA-PLAN_CACHE">
> + <title>PRAGMA PLAN_CACHE</title>
> +
> + <para>
> + The plan cache behavior can be controlled using
> + <literal>PRAGMA PLAN_CACHE</>. This <literal>PRAGMA</> can be used both
> + for whole function or in individual blocks. The following options are
*functions
> + possible: <literal>DEFAULT</literal> - default
> + <application>PL/pgSQL</application> implementation - the system tries
> + to decide between custom plan and generic plan after five query
> + executions, <literal>FORCE_CUSTOM_PLAN</literal> - the chosen execution
> + plan will be the one shot plan - it is specific for every set of
> + used paramaters, <literal>FORCE_GENERIC_PLAN</literal> - the generic
> + plan will be used from the start.
I don't think it's a good idea to explain this here, this'll just get
outdated. I think we should rather have a link here.
> + </para>
> +
> + <para>
> + <indexterm>
> + <primary>PRAGMA PLAN_CACHE</>
> + <secondary>in PL/pgSQL</>
> + </indexterm>
> + The plan for <command>INSERT</command> is always a generic
> plan.
That's this specific insert, right? Should be mentioned, sounds more
generic to me.
> +/* ----------
> + * Returns pointer to current compiler settings
> + * ----------
> + */
> +PLpgSQL_settings *
> +plpgsql_current_settings(void)
> +{
> + return current_settings;
> +}
> +
> +
> +/* ----------
> + * Setup default compiler settings
> + * ----------
> + */
> +void
> +plpgsql_settings_init(PLpgSQL_settings *settings)
> +{
> + current_settings = settings;
> +}
Hm. This is only ever set to &default_settings.
> +/* ----------
> + * Set compiler settings
> + * ----------
> + */
> +void
> +plpgsql_settings_set(PLpgSQL_settings *settings)
> +{
> + PLpgSQL_nsitem *ns_cur = ns_top;
> +
> + /*
> + * Modify settings directly, when ns has local settings data.
> + * When ns uses shared settings, create settings first.
> + */
> + while (ns_cur->itemtype != PLPGSQL_NSTYPE_LABEL)
> + ns_cur = ns_cur->prev;
> +
> + if (ns_cur->local_settings == NULL)
> + {
> + ns_cur->local_settings = palloc(sizeof(PLpgSQL_settings));
> + ns_cur->local_settings->prev = current_settings;
> + current_settings = ns_cur->local_settings;
> + }
> +
> + current_settings->cursor_options = settings->cursor_options;
> +}
This seems like a somewhat weird method. Why do we have a global
settings, when we essentially just want to use something in the current
ns?
- Andres
Andres Freund <andres@anarazel.de> writes:
> I'd like some input from other committers whether we want this. I'm
> somewhat doubtful, but don't have particularly strong feelings.
I don't really want to expose the workings of the plancache at user level.
The heuristics it uses certainly need work, but it'll get hard to change
those once there are SQL features depending on it.
Also, as you note, there are debatable design decisions in this particular
patch. There are already a couple of ways in which control knobs can be
attached to plgsql functions (i.e. custom GUCs and the comp_option stuff),
so why is this patch wanting to invent yet another fundamental mechanism?
And I'm not very happy about it imposing a new reserved keyword, either.
A bigger-picture question is why we'd only provide such functionality
in plpgsql, and not for other uses of prepared plans.
Lastly, it doesn't look to me like the test cases prove anything at all
about whether the feature does what it's claimed to.
regards, tom lane
On 2017-04-05 17:22:34 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > I'd like some input from other committers whether we want this. I'm > > somewhat doubtful, but don't have particularly strong feelings. > > I don't really want to expose the workings of the plancache at user level. > The heuristics it uses certainly need work, but it'll get hard to change > those once there are SQL features depending on it. > > Also, as you note, there are debatable design decisions in this particular > patch. There are already a couple of ways in which control knobs can be > attached to plgsql functions (i.e. custom GUCs and the comp_option stuff), > so why is this patch wanting to invent yet another fundamental mechanism? > And I'm not very happy about it imposing a new reserved keyword, either. > > A bigger-picture question is why we'd only provide such functionality > in plpgsql, and not for other uses of prepared plans. > > Lastly, it doesn't look to me like the test cases prove anything at all > about whether the feature does what it's claimed to. That echoes my perception - so let's move this to the next CF? It's not like this patch has been pending for very long. - Andres
2017-04-05 22:33 GMT+02:00 Andres Freund <andres@anarazel.de>:
Hi,
I'd like some input from other committers whether we want this. I'm
somewhat doubtful, but don't have particularly strong feelings.
> +
> + <sect2 id="plpgsql-declaration-pragma">
> + <title>Block level PRAGMA</title>
> +
> + <indexterm>
> + <primary>PRAGMA</>
> + <secondary>in PL/pgSQL</>
> + </indexterm>
> +
> + <para>
> + The block level <literal>PRAGMA</literal> allows to change the
> + <application>PL/pgSQL</application> compiler behavior. Currently
> + only <literal>PRAGMA PLAN_CACHE</literal> is supported.
Why are we doing this on a block level?
There are few reasons:
1. it is practical for some cases to mix more plan strategies in one function
a)
FOR IN simple_select
LOOP
ENFORCE ONE SHOT PLANS
BEGIN
.. queries ..
END;
END LOOP;
b)
ENFORCE ONE SHOT PLANS
BEGIN
FOR IN complex query requires one shot plan
LOOP
RETURNS TO DEFAULT PLAN CACHE
BEGIN
.. queries ..
END;
END LOOP;
2. This behave is defined in Ada language, and in PL/SQL too. If we will have autonomous transactions, then we can have a equal functionality
a) run complete function under autonomous transaction
b) run some parts of function (some blocks) under autonomous transaction
It is not necessary, but it can avoid to generate auxiliary functions.
> +<programlisting>
> +CREATE FUNCTION enforce_fresh_plan(_id text) RETURNS boolean AS $$
> +DECLARE
> + PRAGMA PLAN_CACHE(force_custom_plan);
> +BEGIN
> + -- in this block every embedded query uses one shot plan
*plans
> + <sect3 id="PRAGMA-PLAN_CACHE">
> + <title>PRAGMA PLAN_CACHE</title>
> +
> + <para>
> + The plan cache behavior can be controlled using
> + <literal>PRAGMA PLAN_CACHE</>. This <literal>PRAGMA</> can be used both
> + for whole function or in individual blocks. The following options are
*functions
> + possible: <literal>DEFAULT</literal> - default
> + <application>PL/pgSQL</application> implementation - the system tries
> + to decide between custom plan and generic plan after five query
> + executions, <literal>FORCE_CUSTOM_PLAN</literal> - the chosen execution
> + plan will be the one shot plan - it is specific for every set of
> + used paramaters, <literal>FORCE_GENERIC_PLAN</literal> - the generic
> + plan will be used from the start.
I don't think it's a good idea to explain this here, this'll just get
outdated. I think we should rather have a link here.
> + </para>
> +
> + <para>
> + <indexterm>
> + <primary>PRAGMA PLAN_CACHE</>
> + <secondary>in PL/pgSQL</>
> + </indexterm>
> + The plan for <command>INSERT</command> is always a generic
> plan.
That's this specific insert, right? Should be mentioned, sounds more
generic to me.
> +/* ----------
> + * Returns pointer to current compiler settings
> + * ----------
> + */
> +PLpgSQL_settings *
> +plpgsql_current_settings(void)
> +{
> + return current_settings;
> +}
> +
> +
> +/* ----------
> + * Setup default compiler settings
> + * ----------
> + */
> +void
> +plpgsql_settings_init(PLpgSQL_settings *settings)
> +{
> + current_settings = settings;
> +}
Hm. This is only ever set to &default_settings.
> +/* ----------
> + * Set compiler settings
> + * ----------
> + */
> +void
> +plpgsql_settings_set(PLpgSQL_settings *settings)
> +{
> + PLpgSQL_nsitem *ns_cur = ns_top;
> +
> + /*
> + * Modify settings directly, when ns has local settings data.
> + * When ns uses shared settings, create settings first.
> + */
> + while (ns_cur->itemtype != PLPGSQL_NSTYPE_LABEL)
> + ns_cur = ns_cur->prev;
> +
> + if (ns_cur->local_settings == NULL)
> + {
> + ns_cur->local_settings = palloc(sizeof(PLpgSQL_settings));
> + ns_cur->local_settings->prev = current_settings;
> + current_settings = ns_cur->local_settings;
> + }
> +
> + current_settings->cursor_options = settings->cursor_options;
> +}
This seems like a somewhat weird method. Why do we have a global
settings, when we essentially just want to use something in the current
ns?
I am not sure if I understand to question.
This settings is implemented as lazy. If ns has not any own settings, then nothing is done. It requires some global variable, because some ns can be skipped.
My first implementation was 1:1 .. ns:settings - but it add some overhead for any ns although ns has not own settings.
Regards
Pavel
- Andres
2017-04-05 23:22 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
Andres Freund <andres@anarazel.de> writes:
> I'd like some input from other committers whether we want this. I'm
> somewhat doubtful, but don't have particularly strong feelings.
I don't really want to expose the workings of the plancache at user level.
The heuristics it uses certainly need work, but it'll get hard to change
those once there are SQL features depending on it.
I am very sceptical about enhancing heuristics - but I am open to any proposals.
The advanced users disable a plan cache with dynamic SQL. But this workaround has strong disadvantages:
1. it is vulnerable to SQL injection
2. it is less readable
Also, as you note, there are debatable design decisions in this particular
patch. There are already a couple of ways in which control knobs can be
attached to plgsql functions (i.e. custom GUCs and the comp_option stuff),
so why is this patch wanting to invent yet another fundamental mechanism?
And I'm not very happy about it imposing a new reserved keyword, either.
1.
custom GUC has not local scope - so it doesn't allow precious settings. With PRAGMA I have perfect control what will be impacted.
#option has function scope
2. I'll not introduce a PRAGMA keyword just for this feature. We would to implement autonomous transactions. There was not any objection against this feature. The PRAGMA allows to share PL/SQL syntax and functionality.
A bigger-picture question is why we'd only provide such functionality
in plpgsql, and not for other uses of prepared plans.
It is out of scope of this patch.
Lastly, it doesn't look to me like the test cases prove anything at all
about whether the feature does what it's claimed to.
I can enhance regress tests - currently there are not direct access to these attributes - so the tests can be indirect only :(
Regards
Pavel
regards, tom lane
That echoes my perception - so let's move this to the next CF? It's not
like this patch has been pending for very long.
sure
Regards
Pavel
- Andres
2017-04-06 8:08 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:
2017-04-05 23:22 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:Andres Freund <andres@anarazel.de> writes:
> I'd like some input from other committers whether we want this. I'm
> somewhat doubtful, but don't have particularly strong feelings.
I don't really want to expose the workings of the plancache at user level.
The heuristics it uses certainly need work, but it'll get hard to change
those once there are SQL features depending on it.I am very sceptical about enhancing heuristics - but I am open to any proposals.The advanced users disable a plan cache with dynamic SQL. But this workaround has strong disadvantages:1. it is vulnerable to SQL injection2. it is less readable
Also, as you note, there are debatable design decisions in this particular
patch. There are already a couple of ways in which control knobs can be
attached to plgsql functions (i.e. custom GUCs and the comp_option stuff),
so why is this patch wanting to invent yet another fundamental mechanism?
And I'm not very happy about it imposing a new reserved keyword, either.1.custom GUC has not local scope - so it doesn't allow precious settings. With PRAGMA I have perfect control what will be impacted.#option has function scope2. I'll not introduce a PRAGMA keyword just for this feature. We would to implement autonomous transactions. There was not any objection against this feature. The PRAGMA allows to share PL/SQL syntax and functionality.
A bigger-picture question is why we'd only provide such functionality
in plpgsql, and not for other uses of prepared plans.It is out of scope of this patch.
The scope of this patch can be enhanced - but it is different task because user interface should be different.
Lastly, it doesn't look to me like the test cases prove anything at all
about whether the feature does what it's claimed to.I can enhance regress tests - currently there are not direct access to these attributes - so the tests can be indirect only :(RegardsPavel
regards, tom lane
On 05/04/17 23:22, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: >> I'd like some input from other committers whether we want this. I'm >> somewhat doubtful, but don't have particularly strong feelings. > > I don't really want to expose the workings of the plancache at user level. > The heuristics it uses certainly need work, That's an understatement, there are thousands of plpgsql functions in large installations of PostgreSQL which use EXECUTE everywhere just to avoid current behavior (and that's just what I've seen). -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 04/05/2017 05:41 PM, Andres Freund wrote: > On 2017-04-05 17:22:34 -0400, Tom Lane wrote: >> Andres Freund <andres@anarazel.de> writes: >>> I'd like some input from other committers whether we want this. I'm >>> somewhat doubtful, but don't have particularly strong feelings. >> I don't really want to expose the workings of the plancache at user level. >> The heuristics it uses certainly need work, but it'll get hard to change >> those once there are SQL features depending on it. >> >> Also, as you note, there are debatable design decisions in this particular >> patch. There are already a couple of ways in which control knobs can be >> attached to plgsql functions (i.e. custom GUCs and the comp_option stuff), >> so why is this patch wanting to invent yet another fundamental mechanism? >> And I'm not very happy about it imposing a new reserved keyword, either. >> >> A bigger-picture question is why we'd only provide such functionality >> in plpgsql, and not for other uses of prepared plans. >> >> Lastly, it doesn't look to me like the test cases prove anything at all >> about whether the feature does what it's claimed to. > That echoes my perception - so let's move this to the next CF? It's not > like this patch has been pending for very long. > Or just Return with Feedback. ISTM before we revisit this we need agreement on a design. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
2017-04-06 12:30 GMT+02:00 Andrew Dunstan <andrew.dunstan@2ndquadrant.com>:
On 04/05/2017 05:41 PM, Andres Freund wrote:
> On 2017-04-05 17:22:34 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> I'd like some input from other committers whether we want this. I'm
>>> somewhat doubtful, but don't have particularly strong feelings.
>> I don't really want to expose the workings of the plancache at user level.
>> The heuristics it uses certainly need work, but it'll get hard to change
>> those once there are SQL features depending on it.
>>
>> Also, as you note, there are debatable design decisions in this particular
>> patch. There are already a couple of ways in which control knobs can be
>> attached to plgsql functions (i.e. custom GUCs and the comp_option stuff),
>> so why is this patch wanting to invent yet another fundamental mechanism?
>> And I'm not very happy about it imposing a new reserved keyword, either.
>>
>> A bigger-picture question is why we'd only provide such functionality
>> in plpgsql, and not for other uses of prepared plans.
>>
>> Lastly, it doesn't look to me like the test cases prove anything at all
>> about whether the feature does what it's claimed to.
> That echoes my perception - so let's move this to the next CF? It's not
> like this patch has been pending for very long.
>
Or just Return with Feedback.
ISTM before we revisit this we need agreement on a design.
I am open to any ideas - there are some my start points
1. the possibility to disable plan cache is real request
2. can be useful if we are able to control plan cache inside function - the mix of settings is real case too
3. GUC are useless - nobody would to disable plan cache globally
I like to see any proposals about syntax or implementation.
Using PRAGMA is one variant - introduced by PLpgSQL origin - Ada language. The PRAGMA syntax can be used for PRAGMA autonomous with well known syntax. It scales well - it supports function, block or command level.
I invite any discussion now or in start of new release cycle.
Regards
Pavel
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 4/6/17 14:32, Pavel Stehule wrote: > I like to see any proposals about syntax or implementation. > > Using PRAGMA is one variant - introduced by PLpgSQL origin - Ada > language. The PRAGMA syntax can be used for PRAGMA autonomous with well > known syntax. It scales well - it supports function, block or command > level. I had pragmas implemented in the original autonomous transactions patch (https://www.postgresql.org/message-id/659a2fce-b6ee-06de-05c0-c8ed6a01979e@2ndquadrant.com).However, the difference thereis that the behavior is lexical, specific to plpgsql, whereas here you are really just selecting run time behavior. So a GUC, and also something that could apply to other places, should be considered. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
2017-04-08 2:30 GMT+02:00 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>:
On 4/6/17 14:32, Pavel Stehule wrote:
> I like to see any proposals about syntax or implementation.
>
> Using PRAGMA is one variant - introduced by PLpgSQL origin - Ada
> language. The PRAGMA syntax can be used for PRAGMA autonomous with well
> known syntax. It scales well - it supports function, block or command
> level.
I had pragmas implemented in the original autonomous transactions patch
(https://www.postgresql.org/message-id/659a2fce-b6ee-06de- 05c0-c8ed6a01979e@2ndquadrant. com).
However, the difference there is that the behavior is lexical, specific
to plpgsql, whereas here you are really just selecting run time
behavior. So a GUC, and also something that could apply to other
places, should be considered.
I'll look there - we coordinate work on that.
Regards
Pavel
--
Peter Eisentraut http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan
From
Daniel Gustafsson
Date:
> On 08 Apr 2017, at 09:42, Pavel Stehule <pavel.stehule@gmail.com> wrote: > > 2017-04-08 2:30 GMT+02:00 Peter Eisentraut <peter.eisentraut@2ndquadrant.com <mailto:peter.eisentraut@2ndquadrant.com>>: > On 4/6/17 14:32, Pavel Stehule wrote: > > I like to see any proposals about syntax or implementation. > > > > Using PRAGMA is one variant - introduced by PLpgSQL origin - Ada > > language. The PRAGMA syntax can be used for PRAGMA autonomous with well > > known syntax. It scales well - it supports function, block or command > > level. > > I had pragmas implemented in the original autonomous transactions patch > (https://www.postgresql.org/message-id/659a2fce-b6ee-06de-05c0-c8ed6a01979e@2ndquadrant.com <https://www.postgresql.org/message-id/659a2fce-b6ee-06de-05c0-c8ed6a01979e@2ndquadrant.com>). > However, the difference there is that the behavior is lexical, specific > to plpgsql, whereas here you are really just selecting run time > behavior. So a GUC, and also something that could apply to other > places, should be considered. > > I'll look there - we coordinate work on that. This patch was moved to the now started commitfest, and is marked as “Needs review”. Based on this thread I will however change it to "waiting for author”, since there seems to be some open questions. Has there been any new work done on this towards a new design/patch? cheers ./daniel
2017-09-05 15:01 GMT+02:00 Daniel Gustafsson <daniel@yesql.se>:
> On 08 Apr 2017, at 09:42, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> 2017-04-08 2:30 GMT+02:00 Peter Eisentraut <peter.eisentraut@2ndquadrant.com <mailto:peter.eisentraut@ 2ndquadrant.com>>:
> On 4/6/17 14:32, Pavel Stehule wrote:
> > I like to see any proposals about syntax or implementation.
> >
> > Using PRAGMA is one variant - introduced by PLpgSQL origin - Ada
> > language. The PRAGMA syntax can be used for PRAGMA autonomous with well
> > known syntax. It scales well - it supports function, block or command
> > level.
>
> I had pragmas implemented in the original autonomous transactions patch
> (https://www.postgresql.org/message-id/659a2fce-b6ee-06de- 05c0-c8ed6a01979e@2ndquadrant. com <https://www.postgresql.org/ message-id/659a2fce-b6ee-06de- 05c0-c8ed6a01979e@2ndquadrant. com>).
> However, the difference there is that the behavior is lexical, specific
> to plpgsql, whereas here you are really just selecting run time
> behavior. So a GUC, and also something that could apply to other
> places, should be considered.
>
> I'll look there - we coordinate work on that.
This patch was moved to the now started commitfest, and is marked as “Needs
review”. Based on this thread I will however change it to "waiting for author”,
since there seems to be some open questions. Has there been any new work done
on this towards a new design/patch?
I didn't any work on this patch last months. I hope so this week I reread this thread and I'll check what I do.
There are few but important questions:
1. we want this feature? I hope so we want - because I don't believe to user invisible great solution - and this is simple solution that helps with readability of some complex PL procedures.
2. what syntax we should to use (if we accept this feature)? There was not another proposal if I remember well - The PRAGMA syntax is strong because we can very well specify to range where the plans caching will be explicitly controlled. It is well readable and static.
Regards
Pavel
cheers ./daniel
Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2. what syntax we should to use (if we accept this feature)? There was not
> another proposal if I remember well - The PRAGMA syntax is strong because
> we can very well specify to range where the plans caching will be
> explicitly controlled. It is well readable and static.
The complaint I have about PRAGMA is that it's yet another syntax for
accomplishing pretty much the same thing. If you don't like the GUC
solution, we've already got the "comp_option" syntax for static options
in plpgsql. Sure, that's not too pretty, but that's not a good reason
to invent yet another way to do it.
regards, tom lane
2017-09-05 19:38 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2. what syntax we should to use (if we accept this feature)? There was not
> another proposal if I remember well - The PRAGMA syntax is strong because
> we can very well specify to range where the plans caching will be
> explicitly controlled. It is well readable and static.
The complaint I have about PRAGMA is that it's yet another syntax for
accomplishing pretty much the same thing. If you don't like the GUC
solution, we've already got the "comp_option" syntax for static options
in plpgsql. Sure, that's not too pretty, but that's not a good reason
to invent yet another way to do it.
comp_option has only function scope, what is too limited for this purpose.
I don't prefer GUC for this purpose because you need to do SET/RESET on two places. With GUC the code can looks like:
PERFORM set_config('cachexx', 'off')
FOR r IN SELECT ...
LOOP
PERFORM set_config(' cachexx', 'on')
....
PERFORM set_config('cachexx', 'off')
END LOOP;
PERFORM set_config('cachexx', 'on');
The another reason for inventing PRAGMA syntax to PLpgSQL was support for autonomous transaction and I am thinking so is good idea using same syntax like PL/SQL does.
regards, tom lane
On Tue, Sep 5, 2017 at 1:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > The complaint I have about PRAGMA is that it's yet another syntax for > accomplishing pretty much the same thing. If you don't like the GUC > solution, we've already got the "comp_option" syntax for static options > in plpgsql. Sure, that's not too pretty, but that's not a good reason > to invent yet another way to do it. On the general question of whether we should have something like this, I expressed a lot of doubt when e6faf910d75027bdce7cd0f2033db4e912592bcc first went in about whether that algorithm was really going to work, and nothing has happened since then to remove any of that doubt. It's pretty clear to me from experiences with customer problems that the heuristics we have often fail to do the right thing, either because queries with no hope of benefiting still replan 5 times - which can waste a ton of CPU when there are many different queries in the plan cache and many sessions - or because queries that would benefit in some cases give up on replanning before they hit a case where a parameter-specific plan helps. I don't think we can just indefinitely continue to resist providing manual control over this behavior on the theory that some day we'll fix it. It's been six years and we haven't made any significant progress. In some cases, a long delay without any progress might just point to a lack of effort that should have been applied, but in this case I think it's because the problem is incredibly hard. I think what we ideally want to do is notice whether the new bind variables cause a change in selectivity which is sufficient to justify a re-plan. If we annotated the original plan with markers indicating that it was valid for all values with a frequency of more than X and less than Y, for example, we could cover most cases involving equality; range queries would need some other kind of annotation. However, it's unclear how the planner could produce such annotations, and it's unclear how expensive checking against them would be if we had them. Barring somebody having a brilliant insight about how to make some system that's way better than what we have right now, I think we can't hold out much hope of any better fix than a manual knob. I think a GUC is a decent, though not perfect, mechanism for this. This problem isn't restricted to PL/pgsql; indeed, the cases I've seen have come via prepared queries, not PL/pgsql functions. Even without that, one advantage of a GUC is that they are fairly broadly understood and experienced users understand what they can do with them. They can be set at various different scopes (system, user, database, SET clause for a particular function) and it's relatively convenient to do so. Some kind of PL/pgsql-specific PRAGMA syntax is more likely to be overlooked by users who would actually benefit from it, and also won't cover non-PL/pgsql cases. If we were going to go the PRAGMA route, it would make more sense to me to define that as a way of setting a GUC for the scope of one PL/pgsql block, like PRAGMA SETTING(custom_plan_tries, 0). I think it is in general unfortunate that we don't have a mechanism to change a GUC for the lifespan of one particular query, like this: LET custom_plan_tries = 0 IN SELECT ... I bet a lot of people would find that quite convenient. The problem of needing to set a planner GUC for one particular query is pretty common, and Pavel is absolutely right that having to do SET beforehand and RESET afterward is ugly. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> I don't think we can just indefinitely continue to resist
> providing manual control over this behavior on the theory that some
> day we'll fix it.
That's fair enough. We need to have a discussion about exactly what
the knob does, which is distinct from the question of how you spell
the incantation for twiddling it. I'm dubious that a dumb "force a
custom plan" setting is going to solve all that many cases usefully.
> I think a GUC is a decent, though not perfect, mechanism for this.
> This problem isn't restricted to PL/pgsql; indeed, the cases I've seen
> have come via prepared queries, not PL/pgsql functions.
That's 100% correct, and is actually the best reason not to consider
a PRAGMA (or any other plpgsql-specific mechanism) as the incantation
spelling.
> I think it is in general unfortunate that we don't have a mechanism to
> change a GUC for the lifespan of one particular query, like this:
> LET custom_plan_tries = 0 IN SELECT ...
Hmm. I think the core problem here is that we're trying to control
the plancache, which is a pretty much behind-the-scenes mechanism.
Except in the case of an explicit PREPARE, you can't even see from
SQL that the cache is being used, or when it's used. So part of what
needs to be thought about, if we use the GUC approach, is when the
GUC's value is consulted. If we don't do anything special then
the GUC(s) would be consulted when retrieving plans from the cache,
and changes in their values from one retrieval to the next might
cause funny behavior. Maybe the relevant settings need to be captured
when the plancache entry is made ... not sure.
regards, tom lane
On Wed, Sep 6, 2017 at 11:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > That's fair enough. We need to have a discussion about exactly what > the knob does, which is distinct from the question of how you spell > the incantation for twiddling it. I'm dubious that a dumb "force a > custom plan" setting is going to solve all that many cases usefully. I think what people need is the ability to force the behavior in either direction - either insist on a custom plan, or insist on a generic plan. The former is what you need if the plan stinks, and the latter is what you need if replanning is a waste of effort. I have seen both cases. The latter has been a bigger problem than the former, because the former can be hacked around in various ugly and inefficient ways, but if we're adding a knob I think it should have three settings. There is perhaps an argument for even more configurability, like altering the number of plan tries from 5 to some other value, but I'm not clear that there's any use case for values other than 0, 5, and +infinity. >> I think it is in general unfortunate that we don't have a mechanism to >> change a GUC for the lifespan of one particular query, like this: > >> LET custom_plan_tries = 0 IN SELECT ... > > Hmm. I think the core problem here is that we're trying to control > the plancache, which is a pretty much behind-the-scenes mechanism. > Except in the case of an explicit PREPARE, you can't even see from > SQL that the cache is being used, or when it's used. So part of what > needs to be thought about, if we use the GUC approach, is when the > GUC's value is consulted. If we don't do anything special then > the GUC(s) would be consulted when retrieving plans from the cache, > and changes in their values from one retrieval to the next might > cause funny behavior. Maybe the relevant settings need to be captured > when the plancache entry is made ... not sure. What sort of funny behavior are you concerned about? It seems likely to me that in most cases the GUC will have the same value every time through, but if it doesn't, I'm not sure why we'd need to use the old value rather than the current one. Indeed, if the user changes the GUC from "force custom" to "force generic" and reruns the function, we want the new value to take effect, lest a POLA violation occur. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
I think a GUC is a decent, though not perfect, mechanism for this.
This problem isn't restricted to PL/pgsql; indeed, the cases I've seen
have come via prepared queries, not PL/pgsql functions. Even without
that, one advantage of a GUC is that they are fairly broadly
understood and experienced users understand what they can do with
them. They can be set at various different scopes (system, user,
database, SET clause for a particular function) and it's relatively
convenient to do so. Some kind of PL/pgsql-specific PRAGMA syntax is
more likely to be overlooked by users who would actually benefit from
it, and also won't cover non-PL/pgsql cases. If we were going to go
the PRAGMA route, it would make more sense to me to define that as a
way of setting a GUC for the scope of one PL/pgsql block, like PRAGMA
SETTING(custom_plan_tries, 0). I think it is in general unfortunate
that we don't have a mechanism to change a GUC for the lifespan of one
particular query, like this:
LET custom_plan_tries = 0 IN SELECT ...
I bet a lot of people would find that quite convenient. The problem
of needing to set a planner GUC for one particular query is pretty
common, and Pavel is absolutely right that having to do SET beforehand
and RESET afterward is ugly.
Currently only prepared statement and PLpgSQL uses plan cache, what I know - so some special GUC has sense only for this two environments.
I understand so GUC can be used and has sense when users cannot to modify source code or when they want to apply this change globally.
For PLpgSQL there should be block, or statement level related syntax - PRAGMA is well known alternative from ADA language. Maybe another syntax like
BEGIN SET xxx = 1, yyy = 2 .. END ... theoretically we can introduce block level GUC. I don't like it, because there are successful syntax from ADA, PL/SQL.
Maybe can be useful enhance a PREPARE command to accept second optional parameter for plan cache controlling - I have not idea about syntax.
Regards
Pavel
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
>
> Hmm. I think the core problem here is that we're trying to control
> the plancache, which is a pretty much behind-the-scenes mechanism.
> Except in the case of an explicit PREPARE, you can't even see from
> SQL that the cache is being used, or when it's used. So part of what
> needs to be thought about, if we use the GUC approach, is when the
> GUC's value is consulted. If we don't do anything special then
> the GUC(s) would be consulted when retrieving plans from the cache,
> and changes in their values from one retrieval to the next might
> cause funny behavior. Maybe the relevant settings need to be captured
> when the plancache entry is made ... not sure.
What sort of funny behavior are you concerned about? It seems likely
to me that in most cases the GUC will have the same value every time
through, but if it doesn't, I'm not sure why we'd need to use the old
value rather than the current one. Indeed, if the user changes the
GUC from "force custom" to "force generic" and reruns the function, we
want the new value to take effect, lest a POLA violation occur.
good note - so changing this GUC on session level requires reset plan cache.
I am not against to GUC, and I am not against to PLpgSQL #option. Just, and I am repeating (I am sorry) - these tools are not practical for usage in PLpgSQL. There should be some block level possibility to do some setting.
Regards
Pavel
On 6 September 2017 at 07:43, Robert Haas <robertmhaas@gmail.com> wrote: > LET custom_plan_tries = 0 IN SELECT ... Tom has pointed me at this proposal, since on another thread I asked for something very similar. (No need to reprise that discussion, but I wanted prepared queries to be able to do SET work_mem = X; SELECT). This idea looks a good way forward to me. Since we're all in roughly the same place, I'd like to propose that we proceed with the following syntax... whether or not this precisely solves OP's issue on this thread. 1. Allow SET to set multiple parameters... SET guc1 = x, guc2 = y This looks fairly straightforward 2. Allow a SET to apply only for a single statement SET guc1 = x, guc2 = y FOR stmt e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable Internally a GUC setting already exists for a single use, via GUC_ACTION_SAVE, so we just need to invoke it. Quick prototype seems like it will deliver quite quickly. I couldn't see a reason to use "LET" rather than just "SET" which would be the POLA choice. Thoughts? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 6 September 2017 at 07:43, Robert Haas <robertmhaas@gmail.com> wrote: > LET custom_plan_tries = 0 IN SELECT ... Tom has pointed me at this proposal, since on another thread I asked for something very similar. (No need to reprise that discussion, but I wanted prepared queries to be able to do SET work_mem = X; SELECT). This idea looks a good way forward to me. Since we're all in roughly the same place, I'd like to propose that we proceed with the following syntax... whether or not this precisely solves OP's issue on this thread. 1. Allow SET to set multiple parameters... SET guc1 = x, guc2 = y This looks fairly straightforward 2. Allow a SET to apply only for a single statement SET guc1 = x, guc2 = y FOR stmt e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable Internally a GUC setting already exists for a single use, via GUC_ACTION_SAVE, so we just need to invoke it. Quick prototype seems like it will deliver quite quickly. I couldn't see a reason to use "LET" rather than just "SET" which would be the POLA choice. Thoughts? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
2017-09-08 19:14 GMT+02:00 Simon Riggs <simon@2ndquadrant.com>:
On 6 September 2017 at 07:43, Robert Haas <robertmhaas@gmail.com> wrote:
> LET custom_plan_tries = 0 IN SELECT ...
Tom has pointed me at this proposal, since on another thread I asked
for something very similar. (No need to reprise that discussion, but I
wanted prepared queries to be able to do SET work_mem = X; SELECT).
This idea looks a good way forward to me.
Since we're all in roughly the same place, I'd like to propose that we
proceed with the following syntax... whether or not this precisely
solves OP's issue on this thread.
1. Allow SET to set multiple parameters...
SET guc1 = x, guc2 = y
This looks fairly straightforward
2. Allow a SET to apply only for a single statement
SET guc1 = x, guc2 = y FOR stmt
e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable
Internally a GUC setting already exists for a single use, via
GUC_ACTION_SAVE, so we just need to invoke it.
Quick prototype seems like it will deliver quite quickly. I couldn't
see a reason to use "LET" rather than just "SET" which would be the
POLA choice.
Thoughts?
why not
Pavel
--
Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan
From
Daniel Gustafsson
Date:
> On 08 Sep 2017, at 19:14, Simon Riggs <simon@2ndquadrant.com> wrote: > > On 6 September 2017 at 07:43, Robert Haas <robertmhaas@gmail.com> wrote: > >> LET custom_plan_tries = 0 IN SELECT ... > > Tom has pointed me at this proposal, since on another thread I asked > for something very similar. (No need to reprise that discussion, but I > wanted prepared queries to be able to do SET work_mem = X; SELECT). > This idea looks a good way forward to me. > > Since we're all in roughly the same place, I'd like to propose that we > proceed with the following syntax... whether or not this precisely > solves OP's issue on this thread. > > 1. Allow SET to set multiple parameters... > SET guc1 = x, guc2 = y > This looks fairly straightforward > > 2. Allow a SET to apply only for a single statement > SET guc1 = x, guc2 = y FOR stmt > e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable > Internally a GUC setting already exists for a single use, via > GUC_ACTION_SAVE, so we just need to invoke it. This syntax proposal makes sense, +1. My immediate thought was that the per-statement GUCs were sort of like options, and most options in our syntax are enclosed with (), like: SET (guc1 = x, guc2 = y) FOR SELECT ..; cheers ./daniel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
2017-09-08 21:21 GMT+02:00 Daniel Gustafsson <daniel@yesql.se>:
> On 08 Sep 2017, at 19:14, Simon Riggs <simon@2ndquadrant.com> wrote:
>
> On 6 September 2017 at 07:43, Robert Haas <robertmhaas@gmail.com> wrote:
>
>> LET custom_plan_tries = 0 IN SELECT ...
>
> Tom has pointed me at this proposal, since on another thread I asked
> for something very similar. (No need to reprise that discussion, but I
> wanted prepared queries to be able to do SET work_mem = X; SELECT).
> This idea looks a good way forward to me.
>
> Since we're all in roughly the same place, I'd like to propose that we
> proceed with the following syntax... whether or not this precisely
> solves OP's issue on this thread.
>
> 1. Allow SET to set multiple parameters...
> SET guc1 = x, guc2 = y
> This looks fairly straightforward
>
> 2. Allow a SET to apply only for a single statement
> SET guc1 = x, guc2 = y FOR stmt
> e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable
> Internally a GUC setting already exists for a single use, via
> GUC_ACTION_SAVE, so we just need to invoke it.
This syntax proposal makes sense, +1. My immediate thought was that the
per-statement GUCs were sort of like options, and most options in our syntax
are enclosed with (), like: SET (guc1 = x, guc2 = y) FOR SELECT ..;
we newer support this syntax in combination with SET keyword
see - CREATE FUNCTION command
personally I prefer syntax without FOR keyword - because following keyword must be reserved keyword
SET x = .., y = .. SELECT ... ;
Regards
Pavel
cheers ./daniel
On Fri, Sep 8, 2017 at 2:48 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > > > 2017-09-08 21:21 GMT+02:00 Daniel Gustafsson <daniel@yesql.se>: >> >> > On 08 Sep 2017, at 19:14, Simon Riggs <simon@2ndquadrant.com> wrote: >> > >> > On 6 September 2017 at 07:43, Robert Haas <robertmhaas@gmail.com> wrote: >> > >> >> LET custom_plan_tries = 0 IN SELECT ... >> > >> > Tom has pointed me at this proposal, since on another thread I asked >> > for something very similar. (No need to reprise that discussion, but I >> > wanted prepared queries to be able to do SET work_mem = X; SELECT). >> > This idea looks a good way forward to me. >> > >> > Since we're all in roughly the same place, I'd like to propose that we >> > proceed with the following syntax... whether or not this precisely >> > solves OP's issue on this thread. >> > >> > 1. Allow SET to set multiple parameters... >> > SET guc1 = x, guc2 = y >> > This looks fairly straightforward >> > >> > 2. Allow a SET to apply only for a single statement >> > SET guc1 = x, guc2 = y FOR stmt >> > e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable >> > Internally a GUC setting already exists for a single use, via >> > GUC_ACTION_SAVE, so we just need to invoke it. >> >> This syntax proposal makes sense, +1. My immediate thought was that the >> per-statement GUCs were sort of like options, and most options in our >> syntax >> are enclosed with (), like: SET (guc1 = x, guc2 = y) FOR SELECT ..; > > we newer support this syntax in combination with SET keyword > > see - CREATE FUNCTION command > > personally I prefer syntax without FOR keyword - because following keyword > must be reserved keyword > > SET x = .., y = .. SELECT ... ; This seems pretty ugly from a syntax perspective. We already have 'SET LOCAL', which manages scope to the current transaction. How about SET BLOCK which would set until you've left the current statement block? merlin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Pavel Stehule <pavel.stehule@gmail.com> writes:
> personally I prefer syntax without FOR keyword - because following keyword
> must be reserved keyword
> SET x = .., y = .. SELECT ... ;
Nope. Most of the statement-starting keywords are *not* fully reserved;
they don't need to be as long as they lead off the statement. But this
proposal would break that. We need to put FOR or IN or another
already-fully-reserved keyword after the SET list, or something's going
to bite us.
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
Merlin Moncure <mmoncure@gmail.com> writes:
> We already have 'SET LOCAL', which manages scope to the current
> transaction. How about SET BLOCK which would set until you've left
> the current statement block?
(1) I do not think this approach will play terribly well in any of the
PLs; their notions of statement blocks all differ from whatever we might
think that is at the interactive-SQL-command level.
(2) AIUI the feature request is specifically for a single-statement
variable change, not any larger scope than that.
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
Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan
From
"David G. Johnston"
Date:
Pavel Stehule <pavel.stehule@gmail.com> writes:
> personally I prefer syntax without FOR keyword - because following keyword
> must be reserved keyword
> SET x = .., y = .. SELECT ... ;
Nope. Most of the statement-starting keywords are *not* fully reserved;
they don't need to be as long as they lead off the statement. But this
proposal would break that. We need to put FOR or IN or another
already-fully-reserved keyword after the SET list, or something's going
to bite us.
Just throwing it out there but can we making putting SET inside a CTE work?
David J.
This seems pretty ugly from a syntax perspective.
>
> SET x = .., y = .. SELECT ... ;
We already have 'SET LOCAL', which manages scope to the current
transaction. How about SET BLOCK which would set until you've left
the current statement block?
This is reason why PRAGMA was designed - it can works on function, block, or statement level. But only in block based PL languages.
merlin
2017-09-08 23:09 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
Pavel Stehule <pavel.stehule@gmail.com> writes:
> personally I prefer syntax without FOR keyword - because following keyword
> must be reserved keyword
> SET x = .., y = .. SELECT ... ;
Nope. Most of the statement-starting keywords are *not* fully reserved;
they don't need to be as long as they lead off the statement. But this
proposal would break that. We need to put FOR or IN or another
already-fully-reserved keyword after the SET list, or something's going
to bite us.
the possibility to control plan cache for any command via GUC outside PLpgSQL can introduce lot of question.
What impact will be on PREPARE command and on EXECUTE command?
If we disable plan cache for EXECUTE, should we remove plan from plan cache? ...
Can we have some diagnostic to get info if some command has cached plan or not?
Regards
Pavel
regards, tom lane
On 9/8/17 13:14, Simon Riggs wrote: > 2. Allow a SET to apply only for a single statement > SET guc1 = x, guc2 = y FOR stmt > e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable > Internally a GUC setting already exists for a single use, via > GUC_ACTION_SAVE, so we just need to invoke it. This doesn't read well to me. It indicates to me "make this setting for this query [in case I run it later]", but it does not indicate that the query will be run. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 9/8/17 13:14, Simon Riggs wrote:
>> 2. Allow a SET to apply only for a single statement
>> SET guc1 = x, guc2 = y FOR stmt
>> e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable
>> Internally a GUC setting already exists for a single use, via
>> GUC_ACTION_SAVE, so we just need to invoke it.
> This doesn't read well to me. It indicates to me "make this setting for
> this query [in case I run it later]", but it does not indicate that the
> query will be run.
Robert's original LET ... IN ... syntax proposal might be better from that
standpoint.
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
On Wed, Sep 6, 2017 at 10:43:39AM -0400, Robert Haas wrote: > helps. I don't think we can just indefinitely continue to resist > providing manual control over this behavior on the theory that some > day we'll fix it. It's been six years and we haven't made any > significant progress. In some cases, a long delay without any > progress might just point to a lack of effort that should have been > applied, but in this case I think it's because the problem is > incredibly hard. Add to that, we didn't even document the behavior until last year: commit fab9d1da4a213fab08fe2d263eedf2408bc4a27aAuthor: Bruce Momjian <bruce@momjian.us>Date: Tue Jun 14 16:11:46 2016 -0400 document when PREPARE uses generic plans Also explain how generic plans are created. Link to PREPARE docs fromwire-protocol prepare docs. Reported-by: Jonathan Rogers Discussion: https://www.postgresql.org/message-id/flat/561E749D.4090301%40socialserve.com -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
2017-09-11 14:44 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 9/8/17 13:14, Simon Riggs wrote:
>> 2. Allow a SET to apply only for a single statement
>> SET guc1 = x, guc2 = y FOR stmt
>> e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable
>> Internally a GUC setting already exists for a single use, via
>> GUC_ACTION_SAVE, so we just need to invoke it.
> This doesn't read well to me. It indicates to me "make this setting for
> this query [in case I run it later]", but it does not indicate that the
> query will be run.
Robert's original LET ... IN ... syntax proposal might be better from that
standpoint.
From my perspective Robert's proposal is not targeted to PLpgSQL well, because it doesn't allow to choose granularity.
I am not sure what is result from this discussion:
1. this feature is wanted
2. a opened question is the syntax
I am sure so GUC are not a good design solution for PL/pgSQL. Robert's proposal does thing bit better, but it has sense more for another environments than PLpgSQL - more, it allows more degree of freedom what has sense for PLpgSQL.
There is possibility to introduce new compile option #option to disable plan cache on function scope. Do you think so it is acceptable solution? It is step forward.
Regards
Pavel
regards, tom lane
On Mon, Sep 18, 2017 at 11:46 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > There is possibility to introduce new compile option #option to disable plan > cache on function scope. Do you think so it is acceptable solution? It is > step forward. You can already set a GUC with function scope. I'm not getting your point. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
2017-09-19 18:33 GMT+02:00 Robert Haas <robertmhaas@gmail.com>:
On Mon, Sep 18, 2017 at 11:46 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> There is possibility to introduce new compile option #option to disable plan
> cache on function scope. Do you think so it is acceptable solution? It is
> step forward.
You can already set a GUC with function scope. I'm not getting your point.
yes, it is true. But implementation of #option is limited to PLpgSQL - so there is not any too much questions - GUC is global - there is lot of points:
* what is correct impact on PREPARE
* what is correct impact on EXECUTE
* what should be done if this GUC is changed ..
Regards
Pavel
On Tue, Sep 19, 2017 at 12:45 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: >> You can already set a GUC with function scope. I'm not getting your >> point. > > yes, it is true. But implementation of #option is limited to PLpgSQL - so > there is not any too much questions - GUC is global - there is lot of > points: > > * what is correct impact on PREPARE > * what is correct impact on EXECUTE > * what should be done if this GUC is changed .. For better or for worse, as a project we've settled on GUCs as a way to control behavior. I think it makes more sense to try to apply that option to new behaviors we want to control than to invent some new system. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
2017-09-19 20:37 GMT+02:00 Robert Haas <robertmhaas@gmail.com>:
On Tue, Sep 19, 2017 at 12:45 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> You can already set a GUC with function scope. I'm not getting your
>> point.
>
> yes, it is true. But implementation of #option is limited to PLpgSQL - so
> there is not any too much questions - GUC is global - there is lot of
> points:
>
> * what is correct impact on PREPARE
> * what is correct impact on EXECUTE
> * what should be done if this GUC is changed ..
For better or for worse, as a project we've settled on GUCs as a way
to control behavior. I think it makes more sense to try to apply that
option to new behaviors we want to control than to invent some new
system.
I have nothing against GUC generally - just in this case, I have knowleadge what is expected behave in plpgsql environment and I miss this knowleadge else where, so I am thinking be good start just for plpgsql (where this issue is mentioned often). The some plpgsql limitted implementation is not barier against any another implementation.
On Tue, Sep 19, 2017 at 1:37 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Sep 19, 2017 at 12:45 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: >>> You can already set a GUC with function scope. I'm not getting your >>> point. >> >> yes, it is true. But implementation of #option is limited to PLpgSQL - so >> there is not any too much questions - GUC is global - there is lot of >> points: >> >> * what is correct impact on PREPARE >> * what is correct impact on EXECUTE >> * what should be done if this GUC is changed .. > > For better or for worse, as a project we've settled on GUCs as a way > to control behavior. I think it makes more sense to try to apply that > option to new behaviors we want to control than to invent some new > system. This seems very sensible. We also have infrastructure at the SQL level (SET) to manage the GUC. Tom upthread (for pretty good reasons) extending SET to pl/pgsql specific scoping but TBH I'm struggling as to why we need to implement new syntax for this; the only thing missing is being able to scope SET statements to a code block FWICT. merlin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
2017-09-19 20:49 GMT+02:00 Merlin Moncure <mmoncure@gmail.com>:
On Tue, Sep 19, 2017 at 1:37 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Sep 19, 2017 at 12:45 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>> You can already set a GUC with function scope. I'm not getting your
>>> point.
>>
>> yes, it is true. But implementation of #option is limited to PLpgSQL - so
>> there is not any too much questions - GUC is global - there is lot of
>> points:
>>
>> * what is correct impact on PREPARE
>> * what is correct impact on EXECUTE
>> * what should be done if this GUC is changed ..
>
> For better or for worse, as a project we've settled on GUCs as a way
> to control behavior. I think it makes more sense to try to apply that
> option to new behaviors we want to control than to invent some new
> system.
This seems very sensible.
We also have infrastructure at the SQL level (SET) to manage the GUC.
Tom upthread (for pretty good reasons) extending SET to pl/pgsql
specific scoping but TBH I'm struggling as to why we need to implement
new syntax for this; the only thing missing is being able to scope SET
statements to a code block FWICT.
here is a GUC based patch for plancache controlling. Looks so this code is working.
It is hard to create regress tests. Any ideas?
Regards
Pavel
merlin
Attachment
On Thu, Oct 12, 2017 at 6:31 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > It is hard to create regress tests. Any ideas? Moving to next CF per lack of reviews. -- Michael
Pavel,
* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> here is a GUC based patch for plancache controlling. Looks so this code is
> working.
This really could use a new thread, imv. This thread is a year old and
about a completely different feature than what you've implemented here.
> It is hard to create regress tests. Any ideas?
Honestly, my idea for this would be to add another option to EXPLAIN
which would make it spit out generic and custom plans, or something of
that sort.
Please review your patches before sending them and don't send in patches
which have random unrelated whitespace changes.
> diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
> index ad8a82f1e3..cc99cf6dcc 100644
> --- a/src/backend/utils/cache/plancache.c
> +++ b/src/backend/utils/cache/plancache.c
> @@ -1031,6 +1033,12 @@ choose_custom_plan(CachedPlanSource *plansource, ParamListInfo boundParams)
> if (IsTransactionStmtPlan(plansource))
> return false;
>
> + /* See if settings wants to force the decision */
> + if (plancache_mode & PLANCACHE_FORCE_GENERIC_PLAN)
> + return false;
> + if (plancache_mode & PLANCACHE_FORCE_CUSTOM_PLAN)
> + return true;
Not a big deal, but I probably would have just expanded the conditional
checks against cursor_options (just below) rather than adding
independent if() statements.
> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> index ae22185fbd..4ce275e39d 100644
> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
> @@ -3916,6 +3923,16 @@ static struct config_enum ConfigureNamesEnum[] =
> NULL, NULL, NULL
> },
>
> + {
> + {"plancache_mode", PGC_USERSET, QUERY_TUNING_OTHER,
> + gettext_noop("Forces use of custom or generic plans."),
> + gettext_noop("It can control query plan cache.")
> + },
> + &plancache_mode,
> + PLANCACHE_DEFAULT, plancache_mode_options,
> + NULL, NULL, NULL
> + },
That long description is shorter than the short description. How about:
short: Controls the planner's selection of custom or generic plan.
long: Prepared queries have custom and generic plans and the planner
will attempt to choose which is better; this can be set to override
the default behavior.
(either that, or just ditch the long description)
> diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
> index 87fab19f3c..962895cc1a 100644
> --- a/src/include/utils/plancache.h
> +++ b/src/include/utils/plancache.h
> @@ -143,7 +143,6 @@ typedef struct CachedPlan
> MemoryContext context; /* context containing this CachedPlan */
> } CachedPlan;
>
> -
> extern void InitPlanCache(void);
> extern void ResetPlanCache(void);
>
Random unrelated whitespace change...
This is also missing documentation updates.
Setting to Waiting for Author, but with those changes I would think we
could mark it ready-for-committer, it seems pretty straight-forward to
me and there seems to be general agreement (now) that it's worthwhile to
have.
Thanks!
Stephen
Attachment
On Mon, Jan 22, 2018 at 5:15 PM, Stephen Frost <sfrost@snowman.net> wrote: > Pavel, > > * Pavel Stehule (pavel.stehule@gmail.com) wrote: >> here is a GUC based patch for plancache controlling. Looks so this code is >> working. > > This really could use a new thread, imv. This thread is a year old and > about a completely different feature than what you've implemented here. + if (plancache_mode & PLANCACHE_FORCE_GENERIC_PLAN) + return false; + if (plancache_mode & PLANCACHE_FORCE_CUSTOM_PLAN) + return true; This should be ==, not &. I could explain why & happens to work, but I won't. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2018-01-23 15:35 GMT+01:00 Robert Haas <robertmhaas@gmail.com>:
On Mon, Jan 22, 2018 at 5:15 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Pavel,
>
> * Pavel Stehule (pavel.stehule@gmail.com) wrote:
>> here is a GUC based patch for plancache controlling. Looks so this code is
>> working.
>
> This really could use a new thread, imv. This thread is a year old and
> about a completely different feature than what you've implemented here.
+ if (plancache_mode & PLANCACHE_FORCE_GENERIC_PLAN)
+ return false;
+ if (plancache_mode & PLANCACHE_FORCE_CUSTOM_PLAN)
+ return true;
This should be ==, not &.
I could explain why & happens to work, but I won't.
you have true, thank you
Pavel
2018-01-22 23:15 GMT+01:00 Stephen Frost <sfrost@snowman.net>:
Pavel,
* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> here is a GUC based patch for plancache controlling. Looks so this code is
> working.
This really could use a new thread, imv. This thread is a year old and
about a completely different feature than what you've implemented here.
true, but now it is too late
> It is hard to create regress tests. Any ideas?
Honestly, my idea for this would be to add another option to EXPLAIN
which would make it spit out generic and custom plans, or something of
that sort.
I looked there, but it needs cycle dependency between CachedPlan and PlannedStmt. It needs more code than this patch and code will not be nicer. I try to do some game with prepared statements
Please review your patches before sending them and don't send in patches
which have random unrelated whitespace changes.
> diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/ plancache.c
> index ad8a82f1e3..cc99cf6dcc 100644
> --- a/src/backend/utils/cache/plancache.c
> +++ b/src/backend/utils/cache/plancache.c
> @@ -1031,6 +1033,12 @@ choose_custom_plan(CachedPlanSource *plansource, ParamListInfo boundParams)
> if (IsTransactionStmtPlan(plansource))
> return false;
>
> + /* See if settings wants to force the decision */
> + if (plancache_mode & PLANCACHE_FORCE_GENERIC_PLAN)
> + return false;
> + if (plancache_mode & PLANCACHE_FORCE_CUSTOM_PLAN)
> + return true;
Not a big deal, but I probably would have just expanded the conditional
checks against cursor_options (just below) rather than adding
independent if() statements.
I don't think so proposed change is better - My opinion is not strong - and commiter can change it simply
> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> index ae22185fbd..4ce275e39d 100644
> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
> @@ -3916,6 +3923,16 @@ static struct config_enum ConfigureNamesEnum[] =
> NULL, NULL, NULL
> },
>
> + {
> + {"plancache_mode", PGC_USERSET, QUERY_TUNING_OTHER,
> + gettext_noop("Forces use of custom or generic plans."),
> + gettext_noop("It can control query plan cache.")
> + },
> + &plancache_mode,
> + PLANCACHE_DEFAULT, plancache_mode_options,
> + NULL, NULL, NULL
> + },
That long description is shorter than the short description. How about:
short: Controls the planner's selection of custom or generic plan.
long: Prepared queries have custom and generic plans and the planner
will attempt to choose which is better; this can be set to override
the default behavior.
changed - thank you for text
(either that, or just ditch the long description)
> diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache. h
> index 87fab19f3c..962895cc1a 100644
> --- a/src/include/utils/plancache.h
> +++ b/src/include/utils/plancache.h
> @@ -143,7 +143,6 @@ typedef struct CachedPlan
> MemoryContext context; /* context containing this CachedPlan */
> } CachedPlan;
>
> -
> extern void InitPlanCache(void);
> extern void ResetPlanCache(void);
>
Random unrelated whitespace change...
fixed
This is also missing documentation updates.
I wrote some basic doc, but native speaker should to write more words about used strategies.
Setting to Waiting for Author, but with those changes I would think we
could mark it ready-for-committer, it seems pretty straight-forward to
me and there seems to be general agreement (now) that it's worthwhile to
have.
Thanks!
attached updated patch
Regards
Pavel
Stephen
Attachment
On 2018/01/24 1:08, Pavel Stehule wrote:
>
>
> 2018-01-22 23:15 GMT+01:00 Stephen Frost <sfrost@snowman.net <mailto:sfrost@snowman.net>>:
>
> Pavel,
>
> * Pavel Stehule (pavel.stehule@gmail.com <mailto:pavel.stehule@gmail.com>) wrote:
> > here is a GUC based patch for plancache controlling. Looks so this code is
> > working.
>
> This really could use a new thread, imv. This thread is a year old and
> about a completely different feature than what you've implemented here.
>
>
> true, but now it is too late
>
>
> > It is hard to create regress tests. Any ideas?
>
> Honestly, my idea for this would be to add another option to EXPLAIN
> which would make it spit out generic and custom plans, or something of
> that sort.
>
>
> I looked there, but it needs cycle dependency between CachedPlan and PlannedStmt. It needs more code than this patch
andcode will not be nicer. I try to do some game with prepared statements
>
> Please review your patches before sending them and don't send in patches
> which have random unrelated whitespace changes.
>
> > diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
> > index ad8a82f1e3..cc99cf6dcc 100644
> > --- a/src/backend/utils/cache/plancache.c
> > +++ b/src/backend/utils/cache/plancache.c
> > @@ -1031,6 +1033,12 @@ choose_custom_plan(CachedPlanSource *plansource, ParamListInfo boundParams)
> > if (IsTransactionStmtPlan(plansource))
> > return false;
> >
> > + /* See if settings wants to force the decision */
> > + if (plancache_mode & PLANCACHE_FORCE_GENERIC_PLAN)
> > + return false;
> > + if (plancache_mode & PLANCACHE_FORCE_CUSTOM_PLAN)
> > + return true;
>
> Not a big deal, but I probably would have just expanded the conditional
> checks against cursor_options (just below) rather than adding
> independent if() statements.
>
>
> I don't think so proposed change is better - My opinion is not strong - and commiter can change it simply
>
>
> > diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> > index ae22185fbd..4ce275e39d 100644
> > --- a/src/backend/utils/misc/guc.c
> > +++ b/src/backend/utils/misc/guc.c
> > @@ -3916,6 +3923,16 @@ static struct config_enum ConfigureNamesEnum[] =
> > NULL, NULL, NULL
> > },
> >
> > + {
> > + {"plancache_mode", PGC_USERSET, QUERY_TUNING_OTHER,
> > + gettext_noop("Forces use of custom or generic plans."),
> > + gettext_noop("It can control query plan cache.")
> > + },
> > + &plancache_mode,
> > + PLANCACHE_DEFAULT, plancache_mode_options,
> > + NULL, NULL, NULL
> > + },
>
> That long description is shorter than the short description. How about:
>
> short: Controls the planner's selection of custom or generic plan.
> long: Prepared queries have custom and generic plans and the planner
> will attempt to choose which is better; this can be set to override
> the default behavior.
>
>
> changed - thank you for text
>
>
> (either that, or just ditch the long description)
>
> > diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
> > index 87fab19f3c..962895cc1a 100644
> > --- a/src/include/utils/plancache.h
> > +++ b/src/include/utils/plancache.h
> > @@ -143,7 +143,6 @@ typedef struct CachedPlan
> > MemoryContext context; /* context containing this CachedPlan */
> > } CachedPlan;
> >
> > -
> > extern void InitPlanCache(void);
> > extern void ResetPlanCache(void);
> >
>
> Random unrelated whitespace change...
>
>
> fixed
>
>
> This is also missing documentation updates.
>
>
> I wrote some basic doc, but native speaker should to write more words about used strategies.
>
>
> Setting to Waiting for Author, but with those changes I would think we
> could mark it ready-for-committer, it seems pretty straight-forward to
> me and there seems to be general agreement (now) that it's worthwhile to
> have.
>
> Thanks!
>
>
> attached updated patch
>
> Regards
>
> Pavel
>
>
> Stephen
Hi Pavel,
I tested your patch on a044378ce2f6268a996c8cce2b7bfb5d82b05c90.
I share my results to you.
- Result of patch command
$ patch -p1 < guc-plancache_mode-180123.patch
patching file doc/src/sgml/config.sgml
Hunk #1 succeeded at 4400 (offset 13 lines).
patching file src/backend/utils/cache/plancache.c
patching file src/backend/utils/misc/guc.c
patching file src/include/utils/plancache.h
patching file src/test/regress/expected/plancache.out
patching file src/test/regress/sql/plancache.sql
- Result of regression test
=======================
All 186 tests passed.
=======================
Regards,
Tatsuro Yamada
2018-01-30 9:06 GMT+01:00 Tatsuro Yamada <yamada.tatsuro@lab.ntt.co.jp>:
On 2018/01/24 1:08, Pavel Stehule wrote:
2018-01-22 23:15 GMT+01:00 Stephen Frost <sfrost@snowman.net <mailto:sfrost@snowman.net>>:
Pavel,
* Pavel Stehule (pavel.stehule@gmail.com <mailto:pavel.stehule@gmail.com>) wrote:
> here is a GUC based patch for plancache controlling. Looks so this code is
> working.
This really could use a new thread, imv. This thread is a year old and
about a completely different feature than what you've implemented here.
true, but now it is too late
> It is hard to create regress tests. Any ideas?
Honestly, my idea for this would be to add another option to EXPLAIN
which would make it spit out generic and custom plans, or something of
that sort.
I looked there, but it needs cycle dependency between CachedPlan and PlannedStmt. It needs more code than this patch and code will not be nicer. I try to do some game with prepared statements
Please review your patches before sending them and don't send in patches
which have random unrelated whitespace changes.
> diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plan cache.c
> index ad8a82f1e3..cc99cf6dcc 100644
> --- a/src/backend/utils/cache/plancache.c
> +++ b/src/backend/utils/cache/plancache.c
> @@ -1031,6 +1033,12 @@ choose_custom_plan(CachedPlanSource *plansource, ParamListInfo boundParams)
> if (IsTransactionStmtPlan(plansource))
> return false;
>
> + /* See if settings wants to force the decision */
> + if (plancache_mode & PLANCACHE_FORCE_GENERIC_PLAN)
> + return false;
> + if (plancache_mode & PLANCACHE_FORCE_CUSTOM_PLAN)
> + return true;
Not a big deal, but I probably would have just expanded the conditional
checks against cursor_options (just below) rather than adding
independent if() statements.
I don't think so proposed change is better - My opinion is not strong - and commiter can change it simply
> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> index ae22185fbd..4ce275e39d 100644
> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
> @@ -3916,6 +3923,16 @@ static struct config_enum ConfigureNamesEnum[] =
> NULL, NULL, NULL
> },
>
> + {
> + {"plancache_mode", PGC_USERSET, QUERY_TUNING_OTHER,
> + gettext_noop("Forces use of custom or generic plans."),
> + gettext_noop("It can control query plan cache.")
> + },
> + &plancache_mode,
> + PLANCACHE_DEFAULT, plancache_mode_options,
> + NULL, NULL, NULL
> + },
That long description is shorter than the short description. How about:
short: Controls the planner's selection of custom or generic plan.
long: Prepared queries have custom and generic plans and the planner
will attempt to choose which is better; this can be set to override
the default behavior.
changed - thank you for text
(either that, or just ditch the long description)
> diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache. h
> index 87fab19f3c..962895cc1a 100644
> --- a/src/include/utils/plancache.h
> +++ b/src/include/utils/plancache.h
> @@ -143,7 +143,6 @@ typedef struct CachedPlan
> MemoryContext context; /* context containing this CachedPlan */
> } CachedPlan;
>
> -
> extern void InitPlanCache(void);
> extern void ResetPlanCache(void);
>
Random unrelated whitespace change...
fixed
This is also missing documentation updates.
I wrote some basic doc, but native speaker should to write more words about used strategies.
Setting to Waiting for Author, but with those changes I would think we
could mark it ready-for-committer, it seems pretty straight-forward to
me and there seems to be general agreement (now) that it's worthwhile to
have.
Thanks!
attached updated patch
Regards
Pavel
Stephen
Hi Pavel,
I tested your patch on a044378ce2f6268a996c8cce2b7bfb5d82b05c90.
I share my results to you.
- Result of patch command
$ patch -p1 < guc-plancache_mode-180123.patch
patching file doc/src/sgml/config.sgml
Hunk #1 succeeded at 4400 (offset 13 lines).
patching file src/backend/utils/cache/plancache.c
patching file src/backend/utils/misc/guc.c
patching file src/include/utils/plancache.h
patching file src/test/regress/expected/plancache.out
patching file src/test/regress/sql/plancache.sql
- Result of regression test
=======================
All 186 tests passed.
=======================
Thank you very much
Regards
Pavel
Regards,
Tatsuro Yamada
On 2018-01-23 17:08:56 +0100, Pavel Stehule wrote: > 2018-01-22 23:15 GMT+01:00 Stephen Frost <sfrost@snowman.net>: > > This really could use a new thread, imv. This thread is a year old and > > about a completely different feature than what you've implemented here. > > > > true, but now it is too late At the very least the CF entry could be renamed moved out the procedual language category?
2018-03-01 23:10 GMT+01:00 Andres Freund <andres@anarazel.de>:
On 2018-01-23 17:08:56 +0100, Pavel Stehule wrote:
> 2018-01-22 23:15 GMT+01:00 Stephen Frost <sfrost@snowman.net>:
> > This really could use a new thread, imv. This thread is a year old and
> > about a completely different feature than what you've implemented here.
> >
>
> true, but now it is too late
At the very least the CF entry could be renamed moved out the procedual
language category?
Why not? Have you idea, what category is best?
Regards
Pavel
On 2018-03-02 03:13:04 +0100, Pavel Stehule wrote: > 2018-03-01 23:10 GMT+01:00 Andres Freund <andres@anarazel.de>: > > > On 2018-01-23 17:08:56 +0100, Pavel Stehule wrote: > > > 2018-01-22 23:15 GMT+01:00 Stephen Frost <sfrost@snowman.net>: > > > > This really could use a new thread, imv. This thread is a year old and > > > > about a completely different feature than what you've implemented here. > > > > > > > > > > true, but now it is too late > > > > At the very least the CF entry could be renamed moved out the procedual > > language category? > > > > Why not? Because the patch adds GUCs that don't have a direct connection toprocedual languages? And the patch's topic still says "plpgsql plan cache behave" which surely is misleading. Seems fairly obvious that neither category nor name is particularly descriptive of the current state? > Have you idea, what category is best? Server Features? Misc? And as a title something about "add GUCs to control custom plan logic"? Greetings, Andres Freund
2018-03-02 3:38 GMT+01:00 Andres Freund <andres@anarazel.de>:
On 2018-03-02 03:13:04 +0100, Pavel Stehule wrote:
> 2018-03-01 23:10 GMT+01:00 Andres Freund <andres@anarazel.de>:
>
> > On 2018-01-23 17:08:56 +0100, Pavel Stehule wrote:
> > > 2018-01-22 23:15 GMT+01:00 Stephen Frost <sfrost@snowman.net>:
> > > > This really could use a new thread, imv. This thread is a year old and
> > > > about a completely different feature than what you've implemented here.
> > > >
> > >
> > > true, but now it is too late
> >
> > At the very least the CF entry could be renamed moved out the procedual
> > language category?
> >
>
> Why not?
Because the patch adds GUCs that don't have a direct connection
toprocedual languages? And the patch's topic still says "plpgsql plan
cache behave" which surely is misleading.
Seems fairly obvious that neither category nor name is particularly
descriptive of the current state?
ok
> Have you idea, what category is best?
Server Features? Misc? And as a title something about "add GUCs to
control custom plan logic"?
I'll move it there.
Regards
Pavel
Greetings,
Andres Freund
2018-03-02 3:43 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com>:
2018-03-02 3:38 GMT+01:00 Andres Freund <andres@anarazel.de>:On 2018-03-02 03:13:04 +0100, Pavel Stehule wrote:
> 2018-03-01 23:10 GMT+01:00 Andres Freund <andres@anarazel.de>:
>
> > On 2018-01-23 17:08:56 +0100, Pavel Stehule wrote:
> > > 2018-01-22 23:15 GMT+01:00 Stephen Frost <sfrost@snowman.net>:
> > > > This really could use a new thread, imv. This thread is a year old and
> > > > about a completely different feature than what you've implemented here.
> > > >
> > >
> > > true, but now it is too late
> >
> > At the very least the CF entry could be renamed moved out the procedual
> > language category?
> >
>
> Why not?
Because the patch adds GUCs that don't have a direct connection
toprocedual languages? And the patch's topic still says "plpgsql plan
cache behave" which surely is misleading.
Seems fairly obvious that neither category nor name is particularly
descriptive of the current state?ok
> Have you idea, what category is best?
Server Features? Misc? And as a title something about "add GUCs to
control custom plan logic"?I'll move it there.
done
Pavel
RegardsPavel
Greetings,
Andres Freund
On 23.01.18 17:08, Pavel Stehule wrote: > attached updated patch This appears to be the patch of record in this thread. I think there is general desire for adding a setting like this, and the implementation is simple enough. One change perhaps: How about naming the default setting "auto" instead of "default". That makes it clearer what it does. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi
2018-07-10 12:01 GMT+02:00 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>:
On 23.01.18 17:08, Pavel Stehule wrote:
> attached updated patch
This appears to be the patch of record in this thread. I think there is
general desire for adding a setting like this, and the implementation is
simple enough.
One change perhaps: How about naming the default setting "auto" instead
of "default". That makes it clearer what it does.
I changed "default" to "auto"
updated patch attached
Regards
Pavel
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On 2018/07/12 18:12, Pavel Stehule wrote: > Hi > > 2018-07-10 12:01 GMT+02:00 Peter Eisentraut <peter.eisentraut@2ndquadrant.com <mailto:peter.eisentraut@2ndquadrant.com>>: > > On 23.01.18 17:08, Pavel Stehule wrote: > > attached updated patch > > This appears to be the patch of record in this thread. I think there is > general desire for adding a setting like this, and the implementation is > simple enough. > > One change perhaps: How about naming the default setting "auto" instead > of "default". That makes it clearer what it does. > > > I changed "default" to "auto" > > updated patch attached > > Regards > > Pavel Hi Pavel, I tested your patch on 40ca70eb. Here is the result. ======================= All 190 tests passed. ======================= Thanks, Tatsuro Yamada
On 12.07.18 11:12, Pavel Stehule wrote: > This appears to be the patch of record in this thread. I think there is > general desire for adding a setting like this, and the implementation is > simple enough. > > One change perhaps: How about naming the default setting "auto" instead > of "default". That makes it clearer what it does. > > > I changed "default" to "auto" > > updated patch attached committed with some editing -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services