[PATCH] Make ExplainBeginGroup()/ExplainEndGroup() public.

Started by Hadi Moshayediover 8 years ago5 messages
#1Hadi Moshayedi
hadi@citusdata.com
1 attachment(s)

Hello,

The attached patch moves declarations of
ExplainBeginGroup()/ExplainEndGroup() from explain.c to explain.h.

This can be useful for extensions that need explain groups in their
custom-scan explain output.

For example, Citus uses groups in its custom explain outputs [1]https://github.com/citusdata/citus/blob/master/src/backend/distributed/planner/multi_explain.c. But it
achieves it by having a copy of
ExplainBeginGroup()/ExplainEndGroup() in its source code, which is not the
best way.

Please review.

[1]: https://github.com/citusdata/citus/blob/master/src/backend/distributed/planner/multi_explain.c
https://github.com/citusdata/citus/blob/master/src/backend/distributed/planner/multi_explain.c

Thanks,
Hadi

Attachments:

make_explainbeginendgroup_public.patchtext/x-patch; charset=US-ASCII; name=make_explainbeginendgroup_public.patchDownload
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 7648201218..46467e1045 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -124,10 +124,6 @@ static void ExplainCustomChildren(CustomScanState *css,
 					  List *ancestors, ExplainState *es);
 static void ExplainProperty(const char *qlabel, const char *value,
 				bool numeric, ExplainState *es);
-static void ExplainOpenGroup(const char *objtype, const char *labelname,
-				 bool labeled, ExplainState *es);
-static void ExplainCloseGroup(const char *objtype, const char *labelname,
-				  bool labeled, ExplainState *es);
 static void ExplainDummyGroup(const char *objtype, const char *labelname,
 				  ExplainState *es);
 static void ExplainXMLTag(const char *tagname, int flags, ExplainState *es);
@@ -3213,7 +3209,7 @@ ExplainPropertyBool(const char *qlabel, bool value, ExplainState *es)
  * If labeled is true, the group members will be labeled properties,
  * while if it's false, they'll be unlabeled objects.
  */
-static void
+void
 ExplainOpenGroup(const char *objtype, const char *labelname,
 				 bool labeled, ExplainState *es)
 {
@@ -3276,7 +3272,7 @@ ExplainOpenGroup(const char *objtype, const char *labelname,
  * Close a group of related objects.
  * Parameters must match the corresponding ExplainOpenGroup call.
  */
-static void
+void
 ExplainCloseGroup(const char *objtype, const char *labelname,
 				  bool labeled, ExplainState *es)
 {
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index 78822b766a..543b2bb0c6 100644
--- a/src/include/commands/explain.h
+++ b/src/include/commands/explain.h
@@ -101,4 +101,9 @@ extern void ExplainPropertyFloat(const char *qlabel, double value, int ndigits,
 extern void ExplainPropertyBool(const char *qlabel, bool value,
 					ExplainState *es);
 
+extern void ExplainOpenGroup(const char *objtype, const char *labelname,
+				 bool labeled, ExplainState *es);
+extern void ExplainCloseGroup(const char *objtype, const char *labelname,
+				  bool labeled, ExplainState *es);
+
 #endif							/* EXPLAIN_H */
#2Hadi Moshayedi
hadi@citusdata.com
In reply to: Hadi Moshayedi (#1)
Re: [PATCH] Make ExplainBeginGroup()/ExplainEndGroup() public.

Title should have been "Make ExplainOpenGroup()/ExplainCloseGroup()
public.".

Sorry for the misspell.

#3Kyotaro HORIGUCHI
horiguchi.kyotaro@lab.ntt.co.jp
In reply to: Hadi Moshayedi (#1)
Re: [PATCH] Make ExplainBeginGroup()/ExplainEndGroup() public.

At Fri, 21 Jul 2017 10:09:25 -0400, Hadi Moshayedi <hadi@citusdata.com> wrote in <CA+_kT_cCew4OKqTL-ueNndEL1jWYu+3e9RdLZP-_WR1KbONzfA@mail.gmail.com>

Hello,

The attached patch moves declarations of
ExplainBeginGroup()/ExplainEndGroup() from explain.c to explain.h.

This can be useful for extensions that need explain groups in their
custom-scan explain output.

For example, Citus uses groups in its custom explain outputs [1]. But it
achieves it by having a copy of
ExplainBeginGroup()/ExplainEndGroup() in its source code, which is not the
best way.

Please review.

[1]
https://github.com/citusdata/citus/blob/master/src/backend/distributed/planner/multi_explain.c

The patch is a kind of straightforward and looks fine for me.

I think they ought to be public, like many ExplainProperty*()
functions. On the other hand this patch can cause symbol
conflicts with some external modules but I think such breakage
doesn't matter so much.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Robert Haas
robertmhaas@gmail.com
In reply to: Kyotaro HORIGUCHI (#3)
Re: [PATCH] Make ExplainBeginGroup()/ExplainEndGroup() public.

On Tue, Jul 25, 2017 at 9:54 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

The patch is a kind of straightforward and looks fine for me.

+1 for this change.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#4)
Re: [PATCH] Make ExplainBeginGroup()/ExplainEndGroup() public.

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Jul 25, 2017 at 9:54 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

The patch is a kind of straightforward and looks fine for me.

+1 for this change.

LGTM too, pushed.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers