Re: Refactoring postgres_fdw/connection.c

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