Re: Refactoring postgres_fdw/connection.c

From: Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>
To: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Refactoring postgres_fdw/connection.c
Date: 2022-07-26 09:33:04
Message-ID: 7e71d7a9-54e1-79c0-f07c-943a0849b3b8@oss.nttdata.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dilip Kumar 2022-07-26 09:33:33 Re: Perform streaming logical transactions by background workers and parallel apply
Previous Message shiy.fnst@fujitsu.com 2022-07-26 09:24:58 RE: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns