Re: making EXPLAIN extensible

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