Re: Refactoring postgres_fdw/connection.c - Mailing list pgsql-hackers

On Tue, Jul 26, 2022 at 4:25 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> At Tue, 26 Jul 2022 00:54:47 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
> > There are two functions, pgfdw_get_result() and
> > pgfdw_get_cleanup_result(),
> > to get a query result. They have almost the same code, call
> > PQisBusy(),
> > WaitLatchOrSocket(), PQconsumeInput() and PQgetResult() in the loop,
> > but only pgfdw_get_cleanup_result() allows its callers to specify the
> > timeout.
> > 0001 patch transforms pgfdw_get_cleanup_result() to the common
> > function
> > to get a query result and makes pgfdw_get_result() use it instead of
> > its own (duplicate) code. The patch also renames
> > pgfdw_get_cleanup_result()
> > to pgfdw_get_result_timed().
>
> Agree to that refactoring.

+1 for that refactoring.  Here are a few comments about the 0001 patch:

I'm not sure it's a good idea to change the function's name, because
that would make backpatching hard.  To avoid that, how about
introducing a workhorse function for pgfdw_get_result and
pgfdw_get_cleanup_result, based on the latter function as you
proposed, and modifying the two functions so that they call the
workhorse function?

@@ -1599,13 +1572,9 @@ pgfdw_finish_pre_commit_cleanup(List *pending_entries)
        entry = (ConnCacheEntry *) lfirst(lc);

        /* Ignore errors (see notes in pgfdw_xact_callback) */
-       while ((res = PQgetResult(entry->conn)) != NULL)
-       {
-           PQclear(res);
-           /* Stop if the connection is lost (else we'll loop infinitely) */
-           if (PQstatus(entry->conn) == CONNECTION_BAD)
-               break;
-       }
+       pgfdw_get_result_timed(entry->conn, 0, &res, NULL);
+       PQclear(res);

The existing code prevents interruption, but this would change to
allow it.  Do we need this change?

> > pgfdw_xact_callback() and pgfdw_subxact_callback() have similar codes
> > to
> > issue COMMIT or RELEASE SAVEPOINT commands. 0002 patch adds the common
> > function,
> > pgfdw_exec_pre_commit(), for that purpose, and changes those functions
> > so that they use the common one.
>
> I'm not sure the two are similar with each other.  The new function
> pgfdw_exec_pre_commit() looks like a merger of two isolated code paths
> intended to share a seven-line codelet.  I feel the code gets a bit
> harder to understand after the change.  I mildly oppose to this part.

I have to agree with Horiguchi-san, because as mentioned by him, 1)
there isn't enough duplicate code in the two bits to justify merging
them into a single function, and 2) the 0002 patch rather just makes
code complicated.  The current implementation is easy to understand,
so I'd vote for leaving them alone for now.

(I think the introduction of pgfdw_abort_cleanup is good, because that
de-duplicated much code that existed both in pgfdw_xact_callback and
in pgfdw_subxact_callback, which would outweigh the downside of
pgfdw_abort_cleanup that it made code somewhat complicated due to the
logic to handle both the transaction and subtransaction cases within
that single function.  But 0002 is not the case, I think.)

> > pgfdw_finish_pre_commit_cleanup() and
> > pgfdw_finish_pre_subcommit_cleanup()
> > have similar codes to wait for the results of COMMIT or RELEASE
> > SAVEPOINT commands.
> > 0003 patch adds the common function, pgfdw_finish_pre_commit(), for
> > that purpose,
> > and replaces those functions with the common one.
> > That is, pgfdw_finish_pre_commit_cleanup() and
> > pgfdw_finish_pre_subcommit_cleanup()
> > are no longer necessary and 0003 patch removes them.
>
> It gives the same feeling with 0002.

I have to agree with Horiguchi-san on this as well; the existing
single-purpose functions are easy to understand, so I'd vote for
leaving them alone.

Sorry for being late to the party.

Best regards,
Etsuro Fujita