Re: The case when AsyncAppend exists also in the qual of Async ForeignScan

On Fri, Jul 2, 2021 at 10:24 PM Andrey Lepikhov
<a(dot)lepikhov(at)postgrespro(dot)ru> wrote:
> I found one bug. Look at the case:
>
> CREATE TABLE test (x int) PARTITION BY HASH (x);
> CREATE TABLE test_1 (x int);
> CREATE TABLE test_2 (x int);
> CREATE FOREIGN TABLE ftest_1 PARTITION OF test
> FOR VALUES WITH (modulus 2, remainder 0)
> SERVER loopback OPTIONS (table_name 'test_1');
> CREATE FOREIGN TABLE ftest_2 PARTITION OF test
> FOR VALUES WITH (modulus 2, remainder 1)
> SERVER loopback2 OPTIONS (table_name 'test_2');
> INSERT INTO test (SELECT * FROM generate_series(1,10));
> ANALYZE test,test_1,test_2,ftest_1,ftest_2;
>
> EXPLAIN (ANALYZE)
> SELECT * FROM test WHERE x NOT IN (
> SELECT avg(x) FROM test WHERE x > 10
> );
> ERROR: InstrEndLoop called on running node
>
> When I added ', COSTS OFF, SUMMARY OFF, TIMING OFF' this example fall
> down into the SEGFAULT.
>
> I prepared quick fix for this problem (see patch in attachment). Maybe
> it is'nt a best solution but it can speedup search of it.

@@ -7015,6 +7015,21 @@ process_pending_request(AsyncRequest *areq)

fetch_more_data(node);

+ /*
+ * If the request are made by another append we will only prepare connection
+ * for the next query and don't take a tuple immediately. It is needed to
+ * prevent possible recursion into a qual subplan.
+ */
+ if (!fetch)
+ {
+ AppendState *node = (AppendState *) areq->requestor;
+
+ ExecAsyncRequestDone(areq, NULL);
+ node->as_needrequest = bms_add_member(node->as_needrequest,
+ areq->request_index);
+ return;
+ }

I don’t think this is a good idea, because it is pretty inconsistent,
as doing ExecAsyncRequestDone(areq, NULL) means that there are no more
tuples while changing as_needrequest like that means that there is at
least one tuple to return. This would happen to work, but for
example, if we add to the core more sanity checks on AsyncRequests,
this would not work well anymore. I tried to devise a consistent
solution for this issue, but I couldn’t. So I feel inclined to
disable async execution in cases where async-capable nodes access to
subplans (or initplans), for now.

Best regards,
Etsuro Fujita