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
