On 03/04/26 23:09, Lukas Fittl wrote:
>> +static int
>> +auto_explain_split_options(char *rawstring, auto_explain_option *options,
>> + int maxoptions, char **errmsg)
>> +{
>
> I think this function probably has the most complexity due to its
> hand-rolled parsing, so it might be good if someone else takes another
> close look at it.
>
> FWIW, I couldn't find anything wrong from a first read.
>
> Otherwise 0003 looks good to me.
>
I did another review of 0003 (0001 and 0002 are now committed), focusing
on auto_explain_split_options(). I didn't find any issues. The code
coverage looks good - auto_explain_split_options() has nearly 100%
coverage and the new code paths are well exercised by the tests.
Regarding the shared_preload_libraries ordering concern: I don't think
there's much we can do from the GUC perspective, I mean, we're
essentially building an extension dependency chain when
auto_explain.log_extension_options references options from other
extensions. If I configure the GUC with 'plan_advice', I need to ensure
pg_plan_advice is loaded before auto_explain. The documentation makes
this clear.
My concern is about that some cloud providers expose
shared_preload_libraries as a dropdown without user control over
ordering. I can be totally wrong, but it seems to me that in this case,
the provider would need to handle dependencies appropriately or have a
way to let the user define the ordering. Or, a possible improvement
would be a post-configuration validation hook that runs after all
shared_preload_libraries are loaded, allowing deferred validation of
cross-extension dependencies like these EXPLAIN options (I'm wondering
that we can have more extension dependencies in the future, e.g
plan_advice and pg_stat_statements [1])
That said, I think we should proceed with 0003 as-is and revisit this
when real-world usage reveals such problems in practice.
[1] https://www.postgresql.org/message-id/CA%2BTgmobtbB74PTUtCQwUNR8D9ZZwKH5ZptoU30ONm34TR954Nw%40mail.gmail.com
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
