Re: Bypassing cursors in postgres_fdw to enable parallel plans

On Tue, 16 Jun 2026 at 14:58, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> 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".
>
The attached version should work properly.

>
> 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.
>
I added the comment now. I am not sure about the position, please do
suggest if it is better at the end, I just added it to being closest to
fetch_size.

>
> 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
>

I understand your concern and I tried to solve it by passing fsstate now,
also saving a backpointer to the node in active_fsstate to solve the issue
with make_tuple_from_result_row. Since we need to have conn from fsstate, I
am not sure how we can do that if we have only active_fsstate passed to the
function.

> 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.
>
Yes, it is removed in this version of the patch.

>
> + 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.
>
> Changed the comments.

> 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
>

--
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH