Horton

Thread

  1. Let plan_cache_mode to be a little less strict

    Andrei Lepikhov <lepihov@gmail.com> — 2025-07-15T10:53:18Z

    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

  2. Re: Let plan_cache_mode to be a little less strict

    Andy Fan <zhihuifan1213@163.com> — 2025-07-31T10:15:16Z

    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

  3. Re: Let plan_cache_mode to be a little less strict

    Sami Imseih <samimseih@gmail.com> — 2025-07-31T16:52:59Z

    > 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

--
Sami

  4. Re: Let plan_cache_mode to be a little less strict

    Michael Paquier <michael@paquier.xyz> — 2025-07-31T22:56:02Z

    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

  5. Re: Let plan_cache_mode to be a little less strict

    Andrei Lepikhov <lepihov@gmail.com> — 2025-08-01T13:07:08Z

    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

  6. Re: Let plan_cache_mode to be a little less strict

    Michael Paquier <michael@paquier.xyz> — 2025-08-04T04:23:26Z

    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

  7. Re: Let plan_cache_mode to be a little less strict

    Andrei Lepikhov <lepihov@gmail.com> — 2025-08-12T11:22:41Z

    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

  8. Re: Let plan_cache_mode to be a little less strict

    Andrei Lepikhov <lepihov@gmail.com> — 2025-08-22T11:53:26Z

    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