On 19/3/2025 18:41, Sami Imseih wrote:
>> On Wed, Mar 19, 2025 at 11:38 AM Sami Imseih <samimseih@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
