Thread: Refactoring postgres_fdw/connection.c
Hi, When reviewing the postgres_fdw parallel-abort patch [1], I found that there are several duplicate codes in postgres_fdw/connection.c. Which seems to make it harder to review the patch changing connection.c. So I'd like to remove such duplicate codes and refactor the functions in connection.c. I attached the following three patches. 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(). 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. 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. [1] https://commitfest.postgresql.org/38/3392/ Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Attachment
At Tue, 26 Jul 2022 00:54:47 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in > Hi, > > When reviewing the postgres_fdw parallel-abort patch [1], I found that > there are several duplicate codes in postgres_fdw/connection.c. > Which seems to make it harder to review the patch changing > connection.c. > So I'd like to remove such duplicate codes and refactor the functions > in connection.c. I attached the following three patches. > > 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. And it looks fine to me. > 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. > 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. Considering that pending_deallocate becomes non-NIL only when toplevel, 38 lines out of 66 lines of the function are the toplevel-dedicated stuff. > [1] https://commitfest.postgresql.org/38/3392/ regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On 2022/07/26 16:25, Kyotaro Horiguchi wrote: > Agree to that refactoring. And it looks fine to me. Thanks for reviewing the patches! > 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. If so, we can pgfdw_exec_pre_commit() into two, one is the common function that sends or executes the command (i.e., calls do_sql_command_begin() or do_sql_command()), and another is the function only for toplevel. The latter function calls the common function and then executes DEALLOCATE ALL things. But this is not the way that other functions like pgfdw_abort_cleanup() is implemented. Those functions have both codes for toplevel and !toplevel (i.e., subxact), and run the processings depending on the argument "toplevel". So I'm thinking that pgfdw_exec_pre_commit() implemented in the same way is better. > It gives the same feeling with 0002. Considering that > pending_deallocate becomes non-NIL only when toplevel, 38 lines out of > 66 lines of the function are the toplevel-dedicated stuff. Same as above. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Fujii-san, On Tue, Jul 26, 2022 at 12:55 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > When reviewing the postgres_fdw parallel-abort patch [1], I found that > there are several duplicate codes in postgres_fdw/connection.c. > Which seems to make it harder to review the patch changing connection.c. > So I'd like to remove such duplicate codes and refactor the functions > in connection.c. I attached the following three patches. > > 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(). > > 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. > > 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. > > [1] https://commitfest.postgresql.org/38/3392/ Thanks for working on this! I'd like to review this after the end of the current CF. Could you add this to the next CF? Best regards, Etsuro Fujita
On 2022/07/26 19:26, Etsuro Fujita wrote: > Thanks for working on this! I'd like to review this after the end of > the current CF. Thanks! > Could you add this to the next CF? Yes. https://commitfest.postgresql.org/39/3782/ Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
At Tue, 26 Jul 2022 18:33:04 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in > > 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. > > If so, we can pgfdw_exec_pre_commit() into two, one is the common > function that sends or executes the command (i.e., calls > do_sql_command_begin() or do_sql_command()), and another is > the function only for toplevel. The latter function calls > the common function and then executes DEALLOCATE ALL things. > > But this is not the way that other functions like > pgfdw_abort_cleanup() > is implemented. Those functions have both codes for toplevel and > !toplevel (i.e., subxact), and run the processings depending > on the argument "toplevel". So I'm thinking that > pgfdw_exec_pre_commit() implemented in the same way is better. I didn't see it from that viewpoint but I don't think that unconditionally justifies other refactoring. If we merge pgfdw_finish_pre_(sub)?commit_cleanup()s this way, in turn pgfdw_subxact_callback() and pgfdw_xact_callback() are going to be almost identical except event IDs to handle. But I don't think we would want to merge them. A concern on 0002 is that it is hiding the subxact-specific steps from the subxact callback. It would look reasonable if it were called from two or more places for each topleve and !toplevel, but actually it has only one caller for each. So I think that pgfdw_exec_pre_commit should not do that and should be renamed to pgfdw_commit_remote() or something. On the other hand pgfdw_finish_pre_commit() hides toplevel-specific steps from the caller so the same argument holds. Another point that makes me concern about the patch is the new function takes an SQL statement, along with the toplevel flag. I guess the reason is that the command for subxact (RELEASE SAVEPOINT %d) requires the current transaction level. However, the values isobtainable very cheap within the cleanup functions. So I propose to get rid of the parameter "sql" from the two functions. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Tue, Jul 26, 2022 at 7:46 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > On 2022/07/26 19:26, Etsuro Fujita wrote: > > Could you add this to the next CF? > > Yes. > https://commitfest.postgresql.org/39/3782/ Thanks! Best regards, Etsuro Fujita
On 2022/07/27 10:36, Kyotaro Horiguchi wrote:
> At Tue, 26 Jul 2022 18:33:04 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>>> 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.
>>
>> If so, we can pgfdw_exec_pre_commit() into two, one is the common
>> function that sends or executes the command (i.e., calls
>> do_sql_command_begin() or do_sql_command()), and another is
>> the function only for toplevel. The latter function calls
>> the common function and then executes DEALLOCATE ALL things.
>>
>> But this is not the way that other functions like
>> pgfdw_abort_cleanup()
>> is implemented. Those functions have both codes for toplevel and
>> !toplevel (i.e., subxact), and run the processings depending
>> on the argument "toplevel". So I'm thinking that
>> pgfdw_exec_pre_commit() implemented in the same way is better.
>
> I didn't see it from that viewpoint but I don't think that
> unconditionally justifies other refactoring. If we merge
> pgfdw_finish_pre_(sub)?commit_cleanup()s this way, in turn
> pgfdw_subxact_callback() and pgfdw_xact_callback() are going to be
> almost identical except event IDs to handle. But I don't think we
> would want to merge them.
I don't think they are so identical because (as you say) they have to handle different event IDs. So I agree we don't
wantto merge them.
> A concern on 0002 is that it is hiding the subxact-specific steps from
> the subxact callback. It would look reasonable if it were called from
> two or more places for each topleve and !toplevel, but actually it has
> only one caller for each. So I think that pgfdw_exec_pre_commit
> should not do that and should be renamed to pgfdw_commit_remote() or
> something. On the other hand pgfdw_finish_pre_commit() hides
> toplevel-specific steps from the caller so the same argument holds.
So you conclusion is to rename pgfdw_exec_pre_commit() to pgfdw_commit_remote() or something?
> Another point that makes me concern about the patch is the new
> function takes an SQL statement, along with the toplevel flag. I guess
> the reason is that the command for subxact (RELEASE SAVEPOINT %d)
> requires the current transaction level. However, the values
> isobtainable very cheap within the cleanup functions. So I propose to
> get rid of the parameter "sql" from the two functions.
Yes, that's possible. That is, pgfdw_exec_pre_commit() can construct the query string by executing the following codes,
insteadof accepting the query as an argument. But one downside of this approach is that the following codes are
executedfor every remote subtransaction entries. Maybe it's cheap to construct the query string as follows, but I'd
liketo avoid any unnecessray overhead if possible. So the patch makes the caller, pgfdw_subxact_callback(), construct
thequery string only once and give it to pgfdw_exec_pre_commit().
curlevel = GetCurrentTransactionNestLevel();
snprintf(sql, sizeof(sql), "RELEASE SAVEPOINT s%d", curlevel);
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
At Thu, 28 Jul 2022 15:26:42 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in > > > On 2022/07/27 10:36, Kyotaro Horiguchi wrote: > > At Tue, 26 Jul 2022 18:33:04 +0900, Fujii Masao > > <masao.fujii@oss.nttdata.com> wrote in > > I didn't see it from that viewpoint but I don't think that > > unconditionally justifies other refactoring. If we merge > > pgfdw_finish_pre_(sub)?commit_cleanup()s this way, in turn > > pgfdw_subxact_callback() and pgfdw_xact_callback() are going to be > > almost identical except event IDs to handle. But I don't think we > > would want to merge them. > > I don't think they are so identical because (as you say) they have to > handle different event IDs. So I agree we don't want to merge them. The xact_callback and subxact_callback handles different sets of event IDs so they can be merged into one switch(). I don't think there's much difference from merging the functions for xact and subxact into one rountine then calling it with a flag to chose one of the two paths. (Even though less-than-half lines of the fuction are shared..) However, still I don't think they ought to be merged. > > A concern on 0002 is that it is hiding the subxact-specific steps from > > the subxact callback. It would look reasonable if it were called from > > two or more places for each topleve and !toplevel, but actually it has > > only one caller for each. So I think that pgfdw_exec_pre_commit > > should not do that and should be renamed to pgfdw_commit_remote() or > > something. On the other hand pgfdw_finish_pre_commit() hides > > toplevel-specific steps from the caller so the same argument holds. > > So you conclusion is to rename pgfdw_exec_pre_commit() to > pgfdw_commit_remote() or something? And the remote stuff is removed from the function. That being said, I don't mean to fight this no longer since that is rather a matter of taste. > > Another point that makes me concern about the patch is the new > > function takes an SQL statement, along with the toplevel flag. I guess > > the reason is that the command for subxact (RELEASE SAVEPOINT %d) > > requires the current transaction level. However, the values > > isobtainable very cheap within the cleanup functions. So I propose to > > get rid of the parameter "sql" from the two functions. > > Yes, that's possible. That is, pgfdw_exec_pre_commit() can construct > the query string by executing the following codes, instead of > accepting the query as an argument. But one downside of this approach > is that the following codes are executed for every remote > subtransaction entries. Maybe it's cheap to construct the query string > as follows, but I'd like to avoid any unnecessray overhead if > possible. So the patch makes the caller, pgfdw_subxact_callback(), > construct the query string only once and give it to > pgfdw_exec_pre_commit(). > > curlevel = GetCurrentTransactionNestLevel(); > snprintf(sql, sizeof(sql), "RELEASE SAVEPOINT s%d", curlevel); That *overhead* has been there and I'm not sure how much actual impact it gives on performance (comparing to the surrounding code). But I would choose leaving it open-coded as-is than turning it into a function that need two tightly-bonded parameters passed and that also tightly bonded to the caller via the parameters. ...In other words, the original code doesn't seem to meet the requirement for a function. However, it's okay if you prefer the functions than the open-coded lines based on the above discussion, I'd stop objecting. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, Jul 28, 2022 at 11:56 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2022/07/27 10:36, Kyotaro Horiguchi wrote: > > At Tue, 26 Jul 2022 18:33:04 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in > >>> 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. > >> > >> If so, we can pgfdw_exec_pre_commit() into two, one is the common > >> function that sends or executes the command (i.e., calls > >> do_sql_command_begin() or do_sql_command()), and another is > >> the function only for toplevel. The latter function calls > >> the common function and then executes DEALLOCATE ALL things. > >> > >> But this is not the way that other functions like > >> pgfdw_abort_cleanup() > >> is implemented. Those functions have both codes for toplevel and > >> !toplevel (i.e., subxact), and run the processings depending > >> on the argument "toplevel". So I'm thinking that > >> pgfdw_exec_pre_commit() implemented in the same way is better. > > > > I didn't see it from that viewpoint but I don't think that > > unconditionally justifies other refactoring. If we merge > > pgfdw_finish_pre_(sub)?commit_cleanup()s this way, in turn > > pgfdw_subxact_callback() and pgfdw_xact_callback() are going to be > > almost identical except event IDs to handle. But I don't think we > > would want to merge them. > > I don't think they are so identical because (as you say) they have to handle different event IDs. So I agree we don't wantto merge them. > > > > A concern on 0002 is that it is hiding the subxact-specific steps from > > the subxact callback. It would look reasonable if it were called from > > two or more places for each topleve and !toplevel, but actually it has > > only one caller for each. So I think that pgfdw_exec_pre_commit > > should not do that and should be renamed to pgfdw_commit_remote() or > > something. On the other hand pgfdw_finish_pre_commit() hides > > toplevel-specific steps from the caller so the same argument holds. > > So you conclusion is to rename pgfdw_exec_pre_commit() to pgfdw_commit_remote() or something? > > > > Another point that makes me concern about the patch is the new > > function takes an SQL statement, along with the toplevel flag. I guess > > the reason is that the command for subxact (RELEASE SAVEPOINT %d) > > requires the current transaction level. However, the values > > isobtainable very cheap within the cleanup functions. So I propose to > > get rid of the parameter "sql" from the two functions. > > Yes, that's possible. That is, pgfdw_exec_pre_commit() can construct the query string by executing the following codes,instead of accepting the query as an argument. But one downside of this approach is that the following codes are executedfor every remote subtransaction entries. Maybe it's cheap to construct the query string as follows, but I'd liketo avoid any unnecessray overhead if possible. So the patch makes the caller, pgfdw_subxact_callback(), construct thequery string only once and give it to pgfdw_exec_pre_commit(). > > curlevel = GetCurrentTransactionNestLevel(); > snprintf(sql, sizeof(sql), "RELEASE SAVEPOINT s%d", curlevel); > Another possibility I can see is that instead of calling pgfdw_exec_pre_commit() (similarly pgfdw_abort_cleanup) for every connection entry, we should call that once from the callback function, and for that we need to move the hash table loop inside that function. The structure of the callback function looks a little fuzzy to me where the same event is checked for every entry of the connection hash table. Instead of simply move that loop should be inside those function (e.g. pgfdw_exec_pre_commit and pgfdw_abort_cleanup), and let called those function called once w.r.t to event and that function should take care of every entry of the connection hash table. The benefit is that we would save a few processing cycles that needed to match events and call the same function for each connection entry. I tried this refactoring in 0004 patch which is not complete, and reattaching other patches too to make CFboat happy. Thoughts? Suggestions? Regards, Amul
Attachment
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
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
Hi Fujii-san,
On Thu, Sep 15, 2022 at 12:17 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
> On 2022/09/05 15:17, Etsuro Fujita wrote:
> > 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.
I agree that if the name of an existing function was bad, we should
rename it, but I do not think the name pgfdw_get_cleanup_result is
bad; I think it is good in the sense that it well represents what the
function waits for.
The patch you proposed changes pgfdw_get_cleanup_result to cover the
timed_out==NULL case, but I do not think it is a good idea to rename
it for such a minor reason. That function is used in all supported
versions, so that would just make back-patching hard.
> > @@ -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?
Yeah, this is intentional; in commit 04e706d42, I coded this to match
the behavior in the non-parallel-commit mode, which does not call
CHECK_FOR_INTERRUPT.
> But could you tell me why?
My concern about doing so is that WaitLatchOrSocket is rather
expensive, so it might lead to useless overhead in most cases.
Anyway, this changes the behavior, so you should show the evidence
that this is useful. I think this would be beyond refactoring,
though.
> > 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()?
Seems like a good idea.
> > 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 :)
Ok, let us discuss this after the parallel-abort patch.
Sorry for the late reply again.
Best regards,
Etsuro Fujita
On 2023/01/29 19:31, Etsuro Fujita wrote: > I agree that if the name of an existing function was bad, we should > rename it, but I do not think the name pgfdw_get_cleanup_result is > bad; I think it is good in the sense that it well represents what the > function waits for. > > The patch you proposed changes pgfdw_get_cleanup_result to cover the > timed_out==NULL case, but I do not think it is a good idea to rename > it for such a minor reason. That function is used in all supported > versions, so that would just make back-patching hard. As far as I understand, the function name pgfdw_get_cleanup_result is used because it's used to get the result during abort cleanup as the comment tells. OTOH new function is used even not during abort clean, e.g., pgfdw_get_result() calls that new function. So I don't think that pgfdw_get_cleanup_result is good name in some places. If you want to leave pgfdw_get_cleanup_result for the existing uses, we can leave that and redefine it so that it just calls the workhorse function pgfdw_get_result_timed. > Yeah, this is intentional; in commit 04e706d42, I coded this to match > the behavior in the non-parallel-commit mode, which does not call > CHECK_FOR_INTERRUPT. > >> But could you tell me why? > > My concern about doing so is that WaitLatchOrSocket is rather > expensive, so it might lead to useless overhead in most cases. pgfdw_get_result() and pgfdw_get_cleanup_result() already call WaitLatchOrSocket() and CHECK_FOR_INTERRUPTS(). That is, during commit phase, they are currently called when receiving the result of COMMIT TRANSACTION command from remote server. Why do we need to worry about their overhead only when executing DEALLOCATE ALL? > Anyway, this changes the behavior, so you should show the evidence > that this is useful. I think this would be beyond refactoring, > though. Isn't it useful to react the interrupts (e.g., shutdown requests) soon even during waiting for the result of DEALLOCATE ALL? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On Tue, Jan 31, 2023 at 3:44 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > On 2023/01/29 19:31, Etsuro Fujita wrote: > > I agree that if the name of an existing function was bad, we should > > rename it, but I do not think the name pgfdw_get_cleanup_result is > > bad; I think it is good in the sense that it well represents what the > > function waits for. > > > > The patch you proposed changes pgfdw_get_cleanup_result to cover the > > timed_out==NULL case, but I do not think it is a good idea to rename > > it for such a minor reason. That function is used in all supported > > versions, so that would just make back-patching hard. > > As far as I understand, the function name pgfdw_get_cleanup_result is > used because it's used to get the result during abort cleanup as > the comment tells. OTOH new function is used even not during abort clean, > e.g., pgfdw_get_result() calls that new function. So I don't think that > pgfdw_get_cleanup_result is good name in some places. Yeah, I agree on that point. > If you want to leave pgfdw_get_cleanup_result for the existing uses, > we can leave that and redefine it so that it just calls the workhorse > function pgfdw_get_result_timed. +1; that's actually what I proposed upthread. :-) BTW the name "pgfdw_get_result_timed" is a bit confusing to me, because the new function works *without* a timeout condition. We usually append the suffix "_internal", so how about "pgfdw_get_result_internal", to avoid that confusion? > > Yeah, this is intentional; in commit 04e706d42, I coded this to match > > the behavior in the non-parallel-commit mode, which does not call > > CHECK_FOR_INTERRUPT. > > > >> But could you tell me why? > > > > My concern about doing so is that WaitLatchOrSocket is rather > > expensive, so it might lead to useless overhead in most cases. > > pgfdw_get_result() and pgfdw_get_cleanup_result() already call > WaitLatchOrSocket() and CHECK_FOR_INTERRUPTS(). That is, during > commit phase, they are currently called when receiving the result > of COMMIT TRANSACTION command from remote server. Why do we need > to worry about their overhead only when executing DEALLOCATE ALL? DEALLOCATE ALL is a light operation and is issued immediately after executing COMMIT TRANSACTION successfully, so I thought that in most cases it too would be likely to be executed successfully and quickly; there would be less need to do so for DEALLOCATE ALL. > > Anyway, this changes the behavior, so you should show the evidence > > that this is useful. I think this would be beyond refactoring, > > though. > > Isn't it useful to react the interrupts (e.g., shutdown requests) > soon even during waiting for the result of DEALLOCATE ALL? That might be useful, but another concern about this is error handling. The existing code (both in parallel commit and non-parallel commit) ignores any kinds of errors in libpq as well as interrupts when doing DEALLOCATE ALL, and then commits the local transaction, making the remote/local transaction states consistent, but IIUC the patch aborts the local transaction when doing the command, e.g., if WaitLatchOrSocket detected some kind of error in socket access, making the transaction states *inconsistent*, which I don't think would be great. So I'm still not sure this would be acceptable. Best regards, Etsuro Fujita