Re: Refactoring postgres_fdw/connection.c

On Tue, Jan 31, 2023 at 3:44 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)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