> The validation point is an interesting one. I agree that we don't
> want the behavior to depend on the order in which options are
> written.
Here is what I applied on top of v6-0001 to correct this issue. Attaching it
as a text file only as Robert may have a different opinion on how to fix
this.
I felt the best way is to create another handler for registering a validation
function. This means we have to loop through the options list twice,
but I don't think that is a problem.
postgres=# explain (remote_plans, analyze) select * from t_r1;
ERROR: EXPLAIN options REMOTE_PLANS and ANALYZE cannot be used together
postgres=# explain (analyze, remote_plans) select * from t_r1;
ERROR: EXPLAIN options REMOTE_PLANS and ANALYZE cannot be used together
Regards,
Sami
From d4350df06cfeb0d9d5d7fe99b898c1a7ef237c97 Mon Sep 17 00:00:00 2001
From: Sami Imseih <sims...@amazon.com>
Date: Thu, 13 Mar 2025 16:15:59 -0500
Subject: [PATCH 1/1] Add a handler to validate an EXPLAIN option defined by an
extension.
---
src/backend/commands/explain_state.c | 28 +++++++++++++++++++++++++++-
src/include/commands/explain_state.h | 11 ++++++++++-
2 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/explain_state.c
b/src/backend/commands/explain_state.c
index 8e6eea20ba8..a052355f3c5 100644
--- a/src/backend/commands/explain_state.c
+++ b/src/backend/commands/explain_state.c
@@ -198,6 +198,14 @@ ParseExplainOptionList(ExplainState *es, List *options,
ParseState *pstate)
/* if the summary was not set explicitly, set default value */
es->summary = (summary_set) ? es->summary : es->analyze;
+
+ /* Validate the extension options */
+ foreach(lc, options)
+ {
+ DefElem *opt = (DefElem *) lfirst(lc);
+
+ ValidateExtensionExplainOption(es, opt);
+ }
}
/*
@@ -320,7 +328,7 @@ ApplyExtensionExplainOption(ExplainState *es, DefElem *opt,
ParseState *pstate)
if (strcmp(ExplainExtensionOptionArray[i].option_name,
opt->defname) == 0)
{
- ExplainExtensionOptionArray[i].option_handler(es, opt,
pstate);
+ ExplainExtensionOptionArray[i].option_handler.apply(es,
opt, pstate);
return true;
}
}
@@ -328,6 +336,24 @@ ApplyExtensionExplainOption(ExplainState *es, DefElem
*opt, ParseState *pstate)
return false;
}
+/*
+ * Validate an EXPLAIN option registered by an extension.
+ *
+ * Should only be called after ApplyExtensionExplainOption.
+ */
+void
+ValidateExtensionExplainOption(ExplainState *es, DefElem *opt)
+{
+ for (int i = 0; i < ExplainExtensionOptionsAssigned; ++i)
+ {
+ if (strcmp(ExplainExtensionOptionArray[i].option_name,
+ opt->defname) == 0)
+ {
+
ExplainExtensionOptionArray[i].option_handler.validate(es, opt);
+ }
+ }
+}
+
/*
* Map the name of an EXPLAIN extension to an integer ID.
*
diff --git a/src/include/commands/explain_state.h
b/src/include/commands/explain_state.h
index 6c161bfb615..6850d4c06b4 100644
--- a/src/include/commands/explain_state.h
+++ b/src/include/commands/explain_state.h
@@ -76,7 +76,15 @@ typedef struct ExplainState
int extension_state_allocated;
} ExplainState;
-typedef void (*ExplainOptionHandler) (ExplainState *, DefElem *, ParseState *);
+typedef void (*ExplainOptionApply) (ExplainState *, DefElem *, ParseState *);
+typedef void (*ExplainOptionValidate) (ExplainState *, DefElem *);
+
+
+typedef struct ExplainOptionHandler
+{
+ ExplainOptionApply apply;
+ ExplainOptionValidate validate;
+} ExplainOptionHandler;
extern ExplainState *NewExplainState(void);
extern void ParseExplainOptionList(ExplainState *es, List *options,
@@ -92,5 +100,6 @@ extern void RegisterExtensionExplainOption(const char
*option_name,
ExplainOptionHandler handler);
extern bool ApplyExtensionExplainOption(ExplainState *es, DefElem *opt,
ParseState *pstate);
+extern void ValidateExtensionExplainOption(ExplainState *es, DefElem *opt);
#endif /* EXPLAIN_STATE_H */
--
2.39.5 (Apple Git-154)
