On Mon, Aug 3, 2026 at 7:28 AM Rafia Sabih <[email protected]> wrote: > Please find the reworked patch set attached. > This version handles the concerns discussed above.
Hi, I noticed while looking over this version of the patch set that there are a lot of similarities between process_pending_request() and save_to_tuplestore(). In one sense, actually, they're exactly the same thing: they track some query that is actively using the connection and whose results must be read before the connection can be used for something else. Possibly they should share code, but for sure we should be suspicious if one of them is called someplace that the other is not. execute_foreign_modify() is one place where things diverge: process_pending_request() is called at the top of the function, and save_to_tuplestore() is called a bit further down. That's a problem, because SQL queries can be executed in the middle, showing that you've put the call to save_to_tuplestore() in the wrong place. Another place where this comes up is pgfdw_exec_query(). There, async requests are handled via process_pending_request(), but for an active scan, we instead Assert that the caller took care of it. But in fact, many callers of pgfdw_exec_query() don't do that (e.g. postgresAnalyzeForeignTable, postgresGetAnalyzeInfoForForeignTable). We either need to make them all do it, or perhaps we should think about changing things so that pgfdw_exec_query() does save_to_tuplestore() itself. That would require every caller to pass a non-NULL PgFdwConnState, or else those that don't would need to be safe for some other reason. Yet another place where this comes up is in fetch_more_data_begin(). It sends a new query, but it doesn't call save_to_tuplestore() first. And it comes up in GetConnection() too, which has pendingAreq handling but no save_to_tuplestore(). It's not impossible to make all of this logic correct if pendingAreq is handled in one way and streaming_fetch is handled in a different way that is also correct but puts all the code in different places. However, it might be easier if we make them as similar as possible, instead of inventing a new way to do a very similar thing. The signature for save_to_tuplestore is still not correct. I don't understand how this can still not be right after as much discussion as we've had. As I said last time: "So this can be simplified down to a one-parameter function: drain_other_active_scan(PgFdwConnState *conn_state)." In the current patch, it has two parameters, which is more than one. Once again, you don't need PGconn *conn. As I said last time: "You don't need the conn parameter either, because it has to be the same as conn_state->active_scan->conn." That's still true. save_to_tuplestore() should be structured with a loop at the top level, instead of entering a loop only after the first call to pgfdw_get_next_result(). Right now, if the first call to pgfdw_get_next_result() returns an unexpected PQresultStatus(), an error will be reported (which is good), but if a later call returns an unexpected PQresultStatus(), no error will be reported, which is bad. Everything that gets done for the first call to pgfdw_get_next_result() should be done for the subsequent ones too. This function currently has three calls to pgfdw_get_next_result() and you want to get that down to no more than two, or possibly just one. The use of #ifdef LIBPQ_HAS_CHUNK_MODE is not correct. If this patch were to be accepted, it would be part of a version of PostgreSQL that definitely has chunk mode. We do not need to cater to the scenario of that not being true. In fetch_more_data(), the call to fetch_stream_result() to clear the remains of the query from the connection is quite scary -- if any tuples were returned there, unexpectedly, they'd be silently dropped. The caller is expecting no tuples, but that's not checked. The bigger picture here is that fetch_stream_result() is highly duplicative of save_to_tuplestore(). The error handling isn't currently identical between those two functions, but it should be. Either we should get rid of fetch_stream_result(), or save_to_tuplestore() should use it as a subroutine. fetch_ct_2 needs to count the number of times that tuples[] has been filled, to a maximum of 2, so that postgresReScanForeignScan can use that information to know how to rewind. But it doesn't currently work that way, because the streaming-tuplestore cases in fetch_more_data() ignore it. If we enter the if (fsstate->tuplestore) block, then we reload tuples[] inside fetch_from_tuplestore() but don't touch fetch_ct_2. If we have no tuplestore, then we call fetch_stream_result() which is fine except when res == NULL. In that case, we return right away without bumping fetch_ct_2. -- Robert Haas EDB: http://www.enterprisedb.com
