On 19/3/2025 18:41, Sami Imseih wrote:
On Wed, Mar 19, 2025 at 11:38 AM Sami Imseih <samims...@gmail.com> wrote:
... as I made the hook signature match that of
ParseExplainOptionList, so both pstate and the options list
are now available to the hook.
This version LGTM, except it's not pgindent-clean.
ugh, sorry about that. ran it for both modified files and it found one
indentation correction from v2.
index 1d4be3c18ac..8fe1ca5c73e 100644
38c38
< + (*explain_validate_options_hook) (es, options, pstate);
---
+ (*explain_validate_options_hook)(es, options, pstate);
59c59
< 2.47.1
v3 attached.
I want to criticize this patch a bit. ;) Why do you think this hook is
not redundant? May we avoid it at all? It seems to perform a too-narrow
task.
For example, if you want to separate parameters that may be used on a
plain EXPLAIN and on EXPLAIN ANALYZE to get behaviour like the following:
EXPLAIN (BUFFERS ON, TIMING ON) SELECT * FROM pg_class;
ERROR: EXPLAIN option TIMING requires ANALYZE
It would be better to add the parameter "type: EXPLAIN_ONLY |
ANALYZE_ONLY | BOTH" to the RegisterExtensionExplainOption() routine.
This value will be saved inside the ExplainExtensionOption structure and
processed by the core inside the ParseExplainOptionList.
> ERROR: EXPLAIN options REMOTE_PLANS and ANALYZE cannot be used
> together
I think the additional parameter covers the case you provided, isn't it?
--
regards, Andrei Lepikhov
