Re: Refactoring postgres_fdw/connection.c

Hi Fujii-san,

On Thu, Sep 15, 2022 at 12:17 AM Fujii Masao
<masao(dot)fujii(at)oss(dot)nttdata(dot)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