Convert planner's AggInfo and AggTransInfo to Nodes - Mailing list pgsql-hackers

I got annoyed just now upon finding that pprint() applied to the planner's
"root" pointer doesn't dump root->agginfos or root->aggtransinfos.  That's
evidently because AggInfo and AggTransInfo aren't proper Nodes, just bare
structs, which presumably is because somebody couldn't be bothered to
write outfuncs support for them.  I'd say that was a questionable shortcut
even when it was made, and there's certainly precious little excuse now
that gen_node_support.pl can do all the heavy lifting.  Hence, PFA a
little finger exercise to turn them into Nodes.  I took the opportunity
to improve related comments too, and in particular to fix some comments
that leave the impression that preprocess_minmax_aggregates still does
its own scan of the query tree.  (It was momentary confusion over that
idea that got me to the point of being annoyed in the first place.)

Any objections so far?

I'm kind of tempted to mount an effort to get rid of as many of
pathnodes.h's "read_write_ignore" annotations as possible.  Some are
necessary to prevent infinite recursion, and others represent considered
judgments that they'd bloat node dumps more than they're worth --- but
I think quite a lot of them arose from plain laziness about updating
outfuncs.c.  With the infrastructure we have now, that's no longer
a good reason.

In particular, I'm tempted to make a dump of PlannerInfo include
all the baserel RelOptInfos (not joins though; there could be a
mighty lot of those.)  I think we didn't print the simple_rel_array[]
array before mostly because outfuncs didn't use to have reasonable
support for printing arrays.

Thoughts?

            regards, tom lane

diff --git a/src/backend/optimizer/plan/planagg.c b/src/backend/optimizer/plan/planagg.c
index 9330908cbf..0f5d8fd978 100644
--- a/src/backend/optimizer/plan/planagg.c
+++ b/src/backend/optimizer/plan/planagg.c
@@ -137,8 +137,8 @@ preprocess_minmax_aggregates(PlannerInfo *root)
         return;

     /*
-     * Scan the tlist and HAVING qual to find all the aggregates and verify
-     * all are MIN/MAX aggregates.  Stop as soon as we find one that isn't.
+     * Examine all the aggregates and verify all are MIN/MAX aggregates.  Stop
+     * as soon as we find one that isn't.
      */
     aggs_list = NIL;
     if (!can_minmax_aggs(root, &aggs_list))
@@ -227,21 +227,21 @@ preprocess_minmax_aggregates(PlannerInfo *root)

 /*
  * can_minmax_aggs
- *        Walk through all the aggregates in the query, and check
- *        if they are all MIN/MAX aggregates.  If so, build a list of the
- *        distinct aggregate calls in the tree.
+ *        Examine all the aggregates in the query, and check if they are
+ *        all MIN/MAX aggregates.  If so, build a list of MinMaxAggInfo
+ *        nodes for them.
  *
  * Returns false if a non-MIN/MAX aggregate is found, true otherwise.
- *
- * This does not descend into subqueries, and so should be used only after
- * reduction of sublinks to subplans.  There mustn't be outer-aggregate
- * references either.
  */
 static bool
 can_minmax_aggs(PlannerInfo *root, List **context)
 {
     ListCell   *lc;

+    /*
+     * This function used to have to scan the query for itself, but now we can
+     * just thumb through the AggInfo list made by preprocess_aggrefs.
+     */
     foreach(lc, root->agginfos)
     {
         AggInfo    *agginfo = (AggInfo *) lfirst(lc);
diff --git a/src/backend/optimizer/prep/prepagg.c b/src/backend/optimizer/prep/prepagg.c
index 404a5f1dac..8f46111186 100644
--- a/src/backend/optimizer/prep/prepagg.c
+++ b/src/backend/optimizer/prep/prepagg.c
@@ -229,7 +229,7 @@ preprocess_aggref(Aggref *aggref, PlannerInfo *root)
     }
     else
     {
-        AggInfo    *agginfo = palloc(sizeof(AggInfo));
+        AggInfo    *agginfo = makeNode(AggInfo);

         agginfo->finalfn_oid = aggfinalfn;
         agginfo->representative_aggref = aggref;
@@ -266,7 +266,7 @@ preprocess_aggref(Aggref *aggref, PlannerInfo *root)
                                         same_input_transnos);
         if (transno == -1)
         {
-            AggTransInfo *transinfo = palloc(sizeof(AggTransInfo));
+            AggTransInfo *transinfo = makeNode(AggTransInfo);

             transinfo->args = aggref->args;
             transinfo->aggfilter = aggref->aggfilter;
@@ -452,7 +452,8 @@ find_compatible_trans(PlannerInfo *root, Aggref *newagg, bool shareable,
     foreach(lc, transnos)
     {
         int            transno = lfirst_int(lc);
-        AggTransInfo *pertrans = (AggTransInfo *) list_nth(root->aggtransinfos, transno);
+        AggTransInfo *pertrans = (AggTransInfo *) list_nth(root->aggtransinfos,
+                                                           transno);

         /*
          * if the transfns or transition state types are not the same then the
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 69ba254372..e650af5ff2 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -442,15 +442,15 @@ struct PlannerInfo
      * Information about aggregates. Filled by preprocess_aggrefs().
      */
     /* AggInfo structs */
-    List       *agginfos pg_node_attr(read_write_ignore);
+    List       *agginfos;
     /* AggTransInfo structs */
-    List       *aggtransinfos pg_node_attr(read_write_ignore);
-    /* number w/ DISTINCT/ORDER BY/WITHIN GROUP */
-    int            numOrderedAggs pg_node_attr(read_write_ignore);
+    List       *aggtransinfos;
+    /* number of aggs with DISTINCT/ORDER BY/WITHIN GROUP */
+    int            numOrderedAggs;
     /* does any agg not support partial mode? */
-    bool        hasNonPartialAggs pg_node_attr(read_write_ignore);
+    bool        hasNonPartialAggs;
     /* is any partial agg non-serializable? */
-    bool        hasNonSerialAggs pg_node_attr(read_write_ignore);
+    bool        hasNonSerialAggs;

     /*
      * These fields are used only when hasRecursion is true:
@@ -3121,6 +3121,10 @@ typedef struct JoinCostWorkspace
  */
 typedef struct AggInfo
 {
+    pg_node_attr(no_copy_equal, no_read)
+
+    NodeTag        type;
+
     /*
      * Link to an Aggref expr this state value is for.
      *
@@ -3129,6 +3133,7 @@ typedef struct AggInfo
      */
     Aggref       *representative_aggref;

+    /* Transition state number for this aggregate */
     int            transno;

     /*
@@ -3137,9 +3142,8 @@ typedef struct AggInfo
      */
     bool        shareable;

-    /* Oid of the final function or InvalidOid */
+    /* Oid of the final function, or InvalidOid if none */
     Oid            finalfn_oid;
-
 } AggInfo;

 /*
@@ -3151,34 +3155,40 @@ typedef struct AggInfo
  */
 typedef struct AggTransInfo
 {
+    pg_node_attr(no_copy_equal, no_read)
+
+    NodeTag        type;
+
+    /* Inputs for this transition state */
     List       *args;
     Expr       *aggfilter;

     /* Oid of the state transition function */
     Oid            transfn_oid;

-    /* Oid of the serialization function or InvalidOid */
+    /* Oid of the serialization function, or InvalidOid if none */
     Oid            serialfn_oid;

-    /* Oid of the deserialization function or InvalidOid */
+    /* Oid of the deserialization function, or InvalidOid if none */
     Oid            deserialfn_oid;

-    /* Oid of the combine function or InvalidOid */
+    /* Oid of the combine function, or InvalidOid if none */
     Oid            combinefn_oid;

     /* Oid of state value's datatype */
     Oid            aggtranstype;
+
+    /* Additional data about transtype */
     int32        aggtranstypmod;
     int            transtypeLen;
     bool        transtypeByVal;
+
+    /* Space-consumption estimate */
     int32        aggtransspace;

-    /*
-     * initial value from pg_aggregate entry
-     */
-    Datum        initValue;
+    /* Initial value from pg_aggregate entry */
+    Datum        initValue pg_node_attr(read_write_ignore);
     bool        initValueIsNull;
-
 } AggTransInfo;

 #endif                            /* PATHNODES_H */