Обсуждение: 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
Вложения
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] 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