RE: Parallel Inserts in CREATE TABLE AS - Mailing list pgsql-hackers

Hi,

I'm very interested in this feature,
and I'm looking at the patch, here are some comments.

1.
+            if (!TupIsNull(outerTupleSlot))
+            {
+                (void) node->ps.dest->receiveSlot(outerTupleSlot, node->ps.dest);
+                node->ps.state->es_processed++;
+            }
+
+            if(TupIsNull(outerTupleSlot))
+                break;
+        }

How about the following style:

        if(TupIsNull(outerTupleSlot))
            Break;
        
        (void) node->ps.dest->receiveSlot(outerTupleSlot, node->ps.dest);
        node->ps.state->es_processed++;

Which looks cleaner.


2.
+
+    if (into != NULL &&
+        IsA(into, IntoClause))
+    {

The check can be replaced by ISCTAS(into).


3.
+    /*
+     * For parallelizing inserts in CTAS i.e. making each
+     * parallel worker inerst it's tuples, we must send
+     * information such as intoclause(for each worker

'inerst' looks like a typo (insert).


4.
+    /* Estimate space for into clause for CTAS. */
+    if (ISCTAS(planstate->intoclause))
+    {
+        intoclausestr = nodeToString(planstate->intoclause);
+        shm_toc_estimate_chunk(&pcxt->estimator, strlen(intoclausestr) + 1);
+        shm_toc_estimate_keys(&pcxt->estimator, 1);
+    }
...
+    if (intoclausestr != NULL)
+    {
+        char *shmptr = (char *)shm_toc_allocate(pcxt->toc,
+                                                strlen(intoclausestr) + 1);
+        strcpy(shmptr, intoclausestr);
+        shm_toc_insert(pcxt->toc, PARALLEL_KEY_INTO_CLAUSE, shmptr);
+    }

The code here call strlen(intoclausestr) for two times,
After checking the existing code in ExecInitParallelPlan,
It used to store the strlen in a variable.

So how about the following style:

    intoclause_len = strlen(intoclausestr);
    ...
    /* Store serialized intoclause. */
    intoclause_space = shm_toc_allocate(pcxt->toc, intoclause_len + 1);
    memcpy(shmptr, intoclausestr, intoclause_len + 1);
    shm_toc_insert(pcxt->toc, PARALLEL_KEY_INTO_CLAUSE, intoclause_space);

the code in ExecInitParallelPlan 


5.
+    if (intoclausestr != NULL)
+    {
+        char *shmptr = (char *)shm_toc_allocate(pcxt->toc,
+                                                strlen(intoclausestr) + 1);
+        strcpy(shmptr, intoclausestr);
+        shm_toc_insert(pcxt->toc, PARALLEL_KEY_INTO_CLAUSE, shmptr);
+    }
+
     /* Set up the tuple queues that the workers will write into. */
-    pei->tqueue = ExecParallelSetupTupleQueues(pcxt, false);
+    if (intoclausestr == NULL)
+        pei->tqueue = ExecParallelSetupTupleQueues(pcxt, false);

The two check about intoclausestr seems can be combined like:

if (intoclausestr != NULL)
{
...
}
else
{
...
}

Best regards,
houzj