Re: Parallel Append implementation

On Wed, Mar 8, 2017 at 2:00 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> Yeah, that seems to be right in most of the cases. The only cases
> where your formula seems to give too few workers is for something like
> : (2, 8, 8). For such subplans, we should at least allocate 8 workers.
> It turns out that in most of the cases in my formula, the Append
> workers allocated is just 1 worker more than the max per-subplan
> worker count. So in (2, 1, 1, 8), it will be a fraction more than 8.
> So in the patch, in addition to the log2() formula you proposed, I
> have made sure that it allocates at least equal to max(per-subplan
> parallel_workers values).

Yeah, I agree with that.

Some review:

+typedef struct ParallelAppendDescData
+{
+    slock_t        pa_mutex;        /* mutual exclusion to choose
next subplan */
+    ParallelAppendInfo pa_info[FLEXIBLE_ARRAY_MEMBER];
+} ParallelAppendDescData;

Instead of having ParallelAppendInfo, how about just int
pa_workers[FLEXIBLE_ARRAY_MEMBER]?  The second structure seems like
overkill, at least for now.

+static inline void
+exec_append_scan_first(AppendState *appendstate)
+{
+    appendstate->as_whichplan = 0;
+}

I don't think this is buying you anything, and suggest backing it out.

+        /* Backward scan is not supported by parallel-aware plans */
+        Assert(!ScanDirectionIsBackward(appendstate->ps.state->es_direction));

I think you could assert ScanDirectionIsForward, couldn't you?
NoMovement, I assume, is right out.

+            elog(DEBUG2, "ParallelAppend : pid %d : all plans already
finished",
+                         MyProcPid);

Please remove (and all similar cases also).

+                 sizeof(*node->as_padesc->pa_info) * node->as_nplans);

I'd use the type name instead.

+    for (i = 0; i < node->as_nplans; i++)
+    {
+        /*
+         * Just setting all the number of workers to 0 is enough. The logic
+         * of choosing the next plan in workers will take care of everything
+         * else.
+         */
+        padesc->pa_info[i].pa_num_workers = 0;
+    }

Here I'd use memset.

+    return (min_whichplan == PA_INVALID_PLAN ? false : true);

Maybe just return (min_whichplan != PA_INVALID_PLAN);

-                                              childrel->cheapest_total_path);
+
childrel->cheapest_total_path);

Unnecessary.

+        {
             partial_subpaths = accumulate_append_subpath(partial_subpaths,
                                        linitial(childrel->partial_pathlist));
+        }

Don't need to add braces.

+            /*
+             * Extract the first unparameterized, parallel-safe one among the
+             * child paths.
+             */

Can we use get_cheapest_parallel_safe_total_inner for this, from
a71f10189dc10a2fe422158a2c9409e0f77c6b9e?

+        if (rel->partial_pathlist != NIL &&
+            (Path *) linitial(rel->partial_pathlist) == subpath)
+            partial_subplans_set = bms_add_member(partial_subplans_set, i);

This seems like a scary way to figure this out.  What if we wanted to
build a parallel append subpath with some path other than the
cheapest, for some reason?  I think you ought to record the decision
that set_append_rel_pathlist makes about whether to use a partial path
or a parallel-safe path, and then just copy it over here.

-                create_append_path(grouped_rel,
-                                   paths,
-                                   NULL,
-                                   0);
+                create_append_path(grouped_rel, paths, NULL, 0);

Unnecessary.

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