Let plan_cache_mode to be a little less strict
Hi,
I believe the behaviour of the choose_custom_plan function should be
adjusted slightly.
The CachedPlanSource can operate in either auto mode or force a specific
plan type. Currently, the force_generic_plan option declares all plans
as generic, except one-shot plans, of course. However, the
CachedPlanSource entry also has a cursor_options field that indicates
the caller's anticipation of a specific plan type for the statement.
This means that the configuration parameter (GUC) currently overrides
any explicit request for a plan type. The documentation does not clearly
explain the rationale behind this, at least to me.
I propose that the declaration of cursor_options should take precedence
over the GUC. This change would alter the interpretation of the GUC from
a 'forced' plan type to a 'default' plan type. By making this
adjustment, users and extensions can be more assured that they will
receive the requested plan type.
If there are no objections, I will add this patch to the next commitfest.
--
regards, Andrei Lepikhov
Attachments:
v0-0001-Make-the-plan-cache-forced-modes-more-flexible.patchtext/plain; charset=UTF-8; name=v0-0001-Make-the-plan-cache-forced-modes-more-flexible.patchDownload+6-7
Andrei Lepikhov <lepihov@gmail.com> writes:
CachedPlanSource entry also has a cursor_options field that indicates
the caller's anticipation of a specific plan type for the statement.
..
I propose that the declaration of cursor_options should take precedence
over the GUC.
This Looks good to me.
--
Best Regards
Andy Fan
This means that the configuration parameter (GUC) currently overrides
any explicit request for a plan type. The documentation does not clearly
explain the rationale behind this, at least to me.
The documentation [0]https://www.postgresql.org/docs/current/spi-spi-prepare.html mentions " If this default behavior is
unsuitable, you can alter it by
passing the CURSOR_OPT_GENERIC_PLAN or CURSOR_OPT_CUSTOM_PLAN flag to
SPI_prepare_cursor". The default behavior here is plan_cache_mode = AUTO.
But the docs leave out the fact that is plan_cache_mode is not AUTO, that the
GUC settings take precedence over the cursor_options. I agree as well that is
wrong. I also agree with your fix.
[0]: https://www.postgresql.org/docs/current/spi-spi-prepare.html
--
Sami
On Thu, Jul 31, 2025 at 11:52:59AM -0500, Sami Imseih wrote:
This means that the configuration parameter (GUC) currently overrides
any explicit request for a plan type. The documentation does not clearly
explain the rationale behind this, at least to me.The documentation [0] mentions " If this default behavior is
unsuitable, you can alter it by
passing the CURSOR_OPT_GENERIC_PLAN or CURSOR_OPT_CUSTOM_PLAN flag to
SPI_prepare_cursor". The default behavior here is plan_cache_mode = AUTO.But the docs leave out the fact that is plan_cache_mode is not AUTO, that the
GUC settings take precedence over the cursor_options. I agree as well that is
wrong. I also agree with your fix.[0] https://www.postgresql.org/docs/current/spi-spi-prepare.html
It seems to me that if we want to keep track of the priority of the
GUC versus the options given to the SPI call, then we should at least
have some tests for this purpose. I would imagine a test module with
a set of SQL functions that act as wrappers of the SPI calls we want
to test, and arguments that can be given directly in input, using
PG_GETARG_POINTER and PG_RETURN_POINTER for some of them. We could
also have a function in regress.c, which may be simpler here. These
functions should be designed to be generic enough, so as they could be
reused for more tests than the ones we'd look at here.
--
Michael
On 1/8/2025 00:56, Michael Paquier wrote:
On Thu, Jul 31, 2025 at 11:52:59AM -0500, Sami Imseih wrote:
But the docs leave out the fact that is plan_cache_mode is not AUTO, that the
GUC settings take precedence over the cursor_options. I agree as well that is
wrong. I also agree with your fix.[0] https://www.postgresql.org/docs/current/spi-spi-prepare.html
It seems to me that if we want to keep track of the priority of the
GUC versus the options given to the SPI call, then we should at least
have some tests for this purpose. I would imagine a test module with
a set of SQL functions that act as wrappers of the SPI calls we want
to test, and arguments that can be given directly in input, using
PG_GETARG_POINTER and PG_RETURN_POINTER for some of them. We could
also have a function in regress.c, which may be simpler here. These
functions should be designed to be generic enough, so as they could be
reused for more tests than the ones we'd look at here.
I considered the worker_spi.c module, which demonstrates various SPI
usage patterns. It might be more beneficial to use this instead of
creating another test module, isn't it?
--
regards, Andrei Lepikhov
On Fri, Aug 01, 2025 at 03:07:08PM +0200, Andrei Lepikhov wrote:
I considered the worker_spi.c module, which demonstrates various SPI usage
patterns. It might be more beneficial to use this instead of creating
another test module, isn't it?
worker_spi is a playground for bgworker tests, so I'm not eager to
make it more complex for the case of cached planned. SQL tests have
more benefits here, IMO, and they should be more efficient and more
predictible. worker_spi can only work with TAP.
--
Michael
On 1/8/2025 00:56, Michael Paquier wrote:
On Thu, Jul 31, 2025 at 11:52:59AM -0500, Sami Imseih wrote:
This means that the configuration parameter (GUC) currently overrides
any explicit request for a plan type. The documentation does not clearly
explain the rationale behind this, at least to me.The documentation [0] mentions " If this default behavior is
unsuitable, you can alter it by
passing the CURSOR_OPT_GENERIC_PLAN or CURSOR_OPT_CUSTOM_PLAN flag to
SPI_prepare_cursor". The default behavior here is plan_cache_mode = AUTO.But the docs leave out the fact that is plan_cache_mode is not AUTO, that the
GUC settings take precedence over the cursor_options. I agree as well that is
wrong. I also agree with your fix.[0] https://www.postgresql.org/docs/current/spi-spi-prepare.html
It seems to me that if we want to keep track of the priority of the
GUC versus the options given to the SPI call, then we should at least
have some tests for this purpose. I would imagine a test module with
a set of SQL functions that act as wrappers of the SPI calls we want
to test, and arguments that can be given directly in input, using
PG_GETARG_POINTER and PG_RETURN_POINTER for some of them.
I'm not sure I understand it clearly. You propose to transfer some
internal data, let's say SPIPlanPtr, between different UDFs?
Reading the documentation, I see that SPI is intended to be used inside
a UDF. For example, the SPI_connect routine 'connect a C function to the
SPI manager'.
I am afraid that this mechanism is not ready to survive a function
context switch safely. Or do you mean it is ok to restrict test coverage
to only the 'saved' statements?
--
regards, Andrei Lepikhov
On 4/8/2025 06:23, Michael Paquier wrote:
On Fri, Aug 01, 2025 at 03:07:08PM +0200, Andrei Lepikhov wrote:
I considered the worker_spi.c module, which demonstrates various SPI usage
patterns. It might be more beneficial to use this instead of creating
another test module, isn't it?worker_spi is a playground for bgworker tests, so I'm not eager to
make it more complex for the case of cached planned. SQL tests have
more benefits here, IMO, and they should be more efficient and more
predictible. worker_spi can only work with TAP.
I began to implement the idea, described above and got stuck on the
issue that we can't explain an SPI plan. It is possible to invent an
'auto_explain' approach (as a TAP test, of course), but it looks wrong.
As an alternative, it is feasible to add to the SPI interface an
SPI_explain_plan routine using the analogy of ExplainExecuteQuery. I
recall hearing some requests for this kind of feature before. Is this a
commitable way?
See the patch attached. Do I understand correctly the 'SPI wrapper'
approach?
--
regards, Andrei Lepikhov