Re: [HACKERS] Parallel Append implementation

On Thu, Aug 31, 2017 at 12:47 PM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> The last updated patch needs a rebase. Attached is the rebased version.
>

Few comments on the first read of the patch:

1.
@@ -279,6 +347,7 @@ voidExecReScanAppend(AppendState *node){ int i;
+ ParallelAppendDesc padesc = node->as_padesc;
 for (i = 0; i < node->as_nplans; i++) {
@@ -298,6 +367,276 @@ ExecReScanAppend(AppendState *node) if (subnode->chgParam == NULL) ExecReScan(subnode); }
+
+ if (padesc)
+ {
+ padesc->pa_first_plan = padesc->pa_next_plan = 0;
+ memset(padesc->pa_finished, 0, sizeof(bool) * node->as_nplans);
+ }
+

For rescan purpose, the parallel state should be reinitialized via
ExecParallelReInitializeDSM.  We need to do that way mainly to avoid
cases where sometimes in rescan machinery we don't perform rescan of
all the nodes.  See commit 41b0dd987d44089dc48e9c70024277e253b396b7.

2.
+ * shared next_subplan counter index to start looking for unfinished plan,

Here using "counter index" seems slightly confusing. I think using one
of those will be better.

3.
+/* ----------------------------------------------------------------
+ * exec_append_leader_next
+ *
+ * To be used only if it's a parallel leader. The backend should scan
+ * backwards from the last plan. This is to prevent it from taking up
+ * the most expensive non-partial plan, i.e. the first subplan.
+ * ----------------------------------------------------------------
+ */
+static bool
+exec_append_leader_next(AppendState *state)

From above explanation, it is clear that you don't want backend to
pick an expensive plan for a leader, but the reason for this different
treatment is not clear.

4.
accumulate_partialappend_subpath()
{
..
+ /* Add partial subpaths, if any. */
+ return list_concat(partial_subpaths, apath_partial_paths);
..
+ return partial_subpaths;
..
+ if (is_partial)
+ return lappend(partial_subpaths, subpath);
..
}

In this function, instead of returning from multiple places
partial_subpaths list, you can just return it at the end and in all
other places just append to it if required.  That way code will look
more clear and simpler.

5.* is created to represent the case that a relation is provably empty.
+ * */typedef struct AppendPath

Spurious line addition.

6.
if (!node->as_padesc)
{
/*
* This is Parallel-aware append. Follow it's own logic of choosing
* the next subplan.
*/
if (!exec_append_seq_next(node))

I think this is the case of non-parallel-aware appends, but the
comments are indicating the opposite.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com