Re: Refactoring postgres_fdw/connection.c

On 2022/09/05 15:17, Etsuro Fujita wrote:
> +1 for that refactoring. Here are a few comments about the 0001 patch:

Thanks for reviewing the 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?

That's possible. We can revive pgfdw_get_cleanup_result() and
make it call pgfdw_get_result_timed(). Also, with the patch,
pgfdw_get_result() already works in that way. But I'm not sure
how much we should be concerned about back-patch "issue"
in this case. We usually rename the internal functions
if new names are better.

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

You imply that we intentially avoided calling CHECK_FOR_INTERRUPT()
there, don't you? But could you tell me why?

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

The function pgfdw_exec_pre_commit() that I newly introduced consists
of two parts; issue the transaction-end command based on
parallel_commit setting and issue DEALLOCATE ALL. The first part is
duplicated between pgfdw_xact_callback() and pgfdw_subxact_callback(),
but the second not (i.e., it's used only by pgfdw_xact_callback()).
So how about getting rid of that non duplicated part from
pgfdw_exec_pre_commit()?

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

Ok, I will reconsider 0003 patch. BTW, parallel abort patch that
you're proposing seems to add new function pgfdw_finish_abort_cleanup()
with the similar structure as the function added by 0003 patch.
So probably it's helpful for us to consider this together :)

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION