Re: Asynchronous Append on postgres_fdw nodes. - Mailing list pgsql-hackers

Hello.

I looked through the nodeAppend.c and postgres_fdw.c part and those
are I think the core of this patch.

-         * figure out which subplan we are currently processing
+         * try to get a tuple from async subplans
+         */
+        if (!bms_is_empty(node->as_needrequest) ||
+            (node->as_syncdone && !bms_is_empty(node->as_asyncpending)))
+        {
+            if (ExecAppendAsyncGetNext(node, &result))
+                return result;

The function ExecAppendAsyncGetNext() is a function called only here,
and contains only 31 lines.  It doesn't seem to me that the separation
makes the code more readable.

-        /* choose new subplan; if none, we're done */
-        if (!node->choose_next_subplan(node))
+        /* wait or poll async events */
+        if (!bms_is_empty(node->as_asyncpending))
+        {
+            Assert(!node->as_syncdone);
+            Assert(bms_is_empty(node->as_needrequest));
+            ExecAppendAsyncEventWait(node);

You moved the function to wait for events from execAsync to
nodeAppend. The former is a generic module that can be used from any
kind of executor nodes, but the latter is specialized for nodeAppend.
In other words, the abstraction level is lowered here.  What is the
reason for the change?


+        /* Perform the actual callback. */
+        ExecAsyncRequest(areq);
+        if (ExecAppendAsyncResponse(areq))
+        {
+            Assert(!TupIsNull(areq->result));
+            *result = areq->result;

Putting aside the name of the functions, the first two function are
used only this way at only two places. ExecAsyncRequest(areq) tells
fdw to store the first tuple among the already received ones to areq,
and ExecAppendAsyncResponse(areq) is checking the result is actually
set. Finally the result is retrieved directory from areq->result.
What is the reason that the two functions are separately exists?


+            /* Perform the actual callback. */
+            ExecAsyncNotify(areq);

Mmm. The usage of the function (or its name) looks completely reverse
to me.  I think FDW should NOTIFY to exec nodes that the new tuple
gets available but the reverse is nonsense. What the function is
actually doing is to REQUEST fdw to fetch tuples that are expected to
have arrived, which is different from what the name suggests.


postgres_fdw.c

> postgresIterateForeignScan(ForeignScanState *node)
> {
>     PgFdwScanState *fsstate = (PgFdwScanState *) node->fdw_state;
>     TupleTableSlot *slot = node->ss.ss_ScanTupleSlot;
> 
>     /*
>      * If this is the first call after Begin or ReScan, we need to create the
>      * cursor on the remote side.
>      */
>     if (!fsstate->cursor_exists)
>         create_cursor(node);

With the patch, cursors are also created in another place so at least
the comment is wrong.  That being said, I think we should unify the
code except the differences between async and sync.  For example, if
the fetch_more_data_begin() needs to be called only for async
fetching, the cursor should be created before calling the function, in
the code path common with sync fetching.


+
+        /* If this was the second part of an async request, we must fetch until NULL. */
+        if (fsstate->async_aware)
+        {
+            /* call once and raise error if not NULL as expected? */
+            while (PQgetResult(conn) != NULL)
+                ;
+            fsstate->conn_state->async_query_sent = false;
+        }

PQgetResult() receives the result of a query at once. This code means
several queries (FETCHes) are queued in, and we discard the result
except the last one.  Actually the res is just PQclear'd just after so
this just discards *all* result of maybe more than one FETCHes.  I
think something's wrong if we need this.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center