Re: de-deduplicate code in DML execution hooks in postgres_fdw

On Thu, Jul 19, 2018 at 2:05 PM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> (2018/04/18 19:34), Ashutosh Bapat wrote:
>>
>> Hi,
>> While working on a fix related to non-direct DML [1], I noticed that
>> postgresExecForeignInsert(), postgresExecForeignUpdate() and
>> postgresExecForeignDelete() functions are almost identical except that
>> postgresExecForeignInsert() doesn't require ctid. The fix that I was
>> working is applicable to Delete and Update but can be useful for
>> Insert as well. I had to add the same code to two places at least and
>> might have missed fixing one of them. Why don't we have a single
>> function which prepares the statement, extract parameters, executes
>> the prepared statement and checks for the results, returned rows etc?
>> It's been a while that these functions are there and haven't produced
>> code which is a lot different for each of these cases. Here's a patch
>> to extract that code into a separate function and use it in all the
>> three hook implementations.
>>
>> [1]
>> https://www.postgresql.org/message-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com
>
>
> +1 for the general idea. (Actually, I also thought the same thing before.)
> But since this is definitely a matter of PG12, ISTM that it's wise to work
> on this after addressing the issue in [1]. My concern is: if we do this
> refactoring now, we might need two patches for fixing the issue in case of
> backpatching as the fix might need to change those executor functions.

The only thing in [1] that would conflict with this patch is the 0002
(and possibly 0001) patch in [2]. We haven't yet decided anything
about whether those patches can be back-patched or not. I think it's
going to take much longer time for the actual solution to arise. But
we don't have to wait for it to commit this patch as well as 0001 and
0002 patches in [2] because a. the larger solution is not likely to be
back-patchable b. it's going to take much longer time. We don't have
any estimate about how much longer it's going to take. We don't want
this small piece to get stuck for that larger piece worst get stuck
indefinitely. So, I will vote for getting it committed ASAP, if we
agree that it's good change. If the solution to [1] happens to require
this refactoring we we can back-patch that since back-patching it
won't hurt.

[1] https://www.postgresql.org/message-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com
[2] https://www.postgresql.org/message-id/CAFjFpRfK69ptCTNChBBk%2BLYMXFzJ92SW6NmG4HLn_1y7xFk%3Dkw%40mail.gmail.com

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company