Re: Bypassing cursors in postgres_fdw to enable parallel plans

On Fri, Jun 5, 2026 at 5:03 AM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com> wrote:
> One of the patch required a rebase, hence attaching the rebased patches.

Did you generate these patches with "git format-patch"? They don't
apply for me, because 0002 contains some of the same changes that are
also present in 0001. I strongly recommend always generating the patch
sets you post from a private branch using "git rebase master" followed
by "git format-patch -v$VERSION master".

You added a new member to FdwScanPrivateIndex which is fine, but you
put it in the middle instead of at the end, which might be OK but is
slightly surprising, and you didn't add a comment on the new member,
which is definitely not OK.

It appears to me that save_to_tuplestore() still has the same problems
that I have mentioned in previous reviews -- maybe not quite as badly,
but to some degree. For example, it still knows about both fsstate and
active_fsstate. Mostly, the references seem to all point to
active_fsstate now, but there is still a comment that refers to just
fsstate, and this whole setup of having two variables like this is
just very error-prone. I think it's really important to get rid of
that. Also, I still think this is wrong:

+ if (active_fsstate->rescan)
+ {
+ if (!PQsendQueryParams(conn, active_fsstate->query,
active_fsstate->numParams,
+ NULL,
active_fsstate->param_values, NULL, NULL, 0))
+ pgfdw_report_error(NULL, conn, active_fsstate->query);

This is exactly what I talked about in
http://postgr.es/m/CA+TgmoZfxpohj1ag8PQyshKpFu+krUgeH0Xd=34Wvn29F5DWhw@mail.gmail.com
but it seems that it's not fixed: we're still sending the query if it
hasn't been sent yet, and we just shouldn't be doing that. I don't
think it's the right idea to have a "bool rescan" flag at all.
postgresReScanForeignScan just sets so that save_to_tuplestore can use
it to send the query when that's not actually needed in the first
place.

+ if (fsstate->conn_state->active_scan &&
+ fsstate != fsstate->conn_state->active_scan)
+ save_to_tuplestore(node);

I don't think this logic is correct. We shouldn't be trying to
initialize the current scan if the current scan is already active. It
seems to me that the only condition here should be the first one -- if
(fsstate_conn_state->active_scan == NULL) save_to_tuplestore(node). If
that's not sufficient, then it implies that init_scan() can be called
in some situation where the scan is already initialized, which is a
bug that should be fixed at its source, rather than here.

I generally recommend avoiding mention of specific details of the C
code in the regression tests. If you do that, as you have done, then
the SQL comments can need to be updated when the C code is changed,
even if the behavior has not changed, which is annoying.

I think that the changes to make_tuple_from_result_row are unlikely to
be correct. Why would this special handling be needed? If we're
passing the correct fsstate and the correct rel, then I don't
understand why the previous logic would produce the wrong answer. I
wonder if this is a holdover from an earlier patch version where
save_to_tuplestore() was more buggy than it is now. Or maybe it still
has a bug that we should fix. But I don't think changing
make_tuple_from_result_row is likely to be the right answer.

--
Robert Haas
EDB: http://www.enterprisedb.com