public inbox for [email protected]  

help / color / mirror / Atom feed

From: David Zhang <[email protected]>
To: Fujii Masao <[email protected]>
To: Etsuro Fujita <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: postgres_fdw: commit remote (sub)transactions in parallel during pre-commit
Date: Fri, 18 Feb 2022 13:55:19 -0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CAPmGK17dAZCXvwnfpr1eTfknTGdt=hYTV9405Gt5SqPOX8K84w@mail.gmail.com>
	<[email protected]>
	<CAPmGK17XczZmzU=0mhk2_oG9Omo1p-uZCNdZAh_3vuC6+YboiA@mail.gmail.com>
	<[email protected]>
	<CAPmGK14rr1R_t4xxWyJVng04nDjGnxi=J5fz-TBNBH9Xn0OFYQ@mail.gmail.com>
	<[email protected]>
	<CAPmGK15FuPVGx3TGHKShsbPKKtF1y58-ZLcKoxfN-nqLj1dZ=g@mail.gmail.com>
	<[email protected]>
	<CAPmGK158hrd=ZfXmgkmNFHivgh18e4oE2Gz151C2Q4OBDjZ08A@mail.gmail.com>
	<[email protected]>
	<CAPmGK17nfEZkKhqoP8fAPKbnfSpf2JVgV5+_Xjv2mn8P-4X0hQ@mail.gmail.com>
	<[email protected]>

Thanks a lot for updating the patch.

Tried to apply the patches to master branch, no warning found and 
regression test passed.

Now, we have many places (5) calling the same function with a constant 
number 30000. Is this a good time to consider redefine this number a 
macro somewhere?

Thank you,

On 2022-02-17 8:46 a.m., Fujii Masao wrote:
>
>
> On 2022/02/11 21:59, Etsuro Fujita wrote:
>> I tweaked comments/docs a little bit as well.
>
> Thanks for updating the patches!
>
> I reviewed 0001 patch. It looks good to me except the following minor 
> things. If these are addressed, I think that the 001 patch can be 
> marked as ready for committer.
>
> +     * Also determine to commit (sub)transactions opened on the 
> remote server
> +     * in parallel at (sub)transaction end.
>
> Like the comment "Determine whether to keep the connection ...", 
> "determine to commit" should be "determine whether to commit"?
>
> "remote server" should be "remote servers"?
>
>
> +    curlevel = GetCurrentTransactionNestLevel();
> +    snprintf(sql, sizeof(sql), "RELEASE SAVEPOINT s%d", curlevel);
>
> Why does pgfdw_finish_pre_subcommit_cleanup() need to call 
> GetCurrentTransactionNestLevel() and construct the "RELEASE SAVEPOINT" 
> query string again? pgfdw_subxact_callback() already does them and 
> probably we can make it pass either of them to 
> pgfdw_finish_pre_subcommit_cleanup() as its argument.
>
>
> +       This option controls whether <filename>postgres_fdw</filename> 
> commits
> +       remote (sub)transactions opened on a foreign server in a local
> +       (sub)transaction in parallel when the local (sub)transaction 
> commits.
>
> "a foreign server" should be "foreign servers"?
>
> "in a local (sub)transaction" part seems not to be necessary.
>
> Regards,
>
-- 
David

Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca

view thread (9+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: postgres_fdw: commit remote (sub)transactions in parallel during pre-commit
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox