Message-ID: <3e57ebd7-b536-333e-c877-49a3ebb57079@oss.nttdata.com>
Date: Tue, 8 Feb 2022 03:49:47 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.5.1
Subject: Re: postgres_fdw: commit remote (sub)transactions in parallel during
 pre-commit
Content-Language: en-US
To: Etsuro Fujita <etsuro.fujita@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <CAPmGK17dAZCXvwnfpr1eTfknTGdt=hYTV9405Gt5SqPOX8K84w@mail.gmail.com>
 <65af8fb4-4c45-9cc3-c6ec-80f3ac488e07@oss.nttdata.com>
 <CAPmGK17XczZmzU=0mhk2_oG9Omo1p-uZCNdZAh_3vuC6+YboiA@mail.gmail.com>
 <95066202-c7e6-e4be-05cc-bdceb45b68c7@oss.nttdata.com>
 <CAPmGK14rr1R_t4xxWyJVng04nDjGnxi=J5fz-TBNBH9Xn0OFYQ@mail.gmail.com>
 <2d7c480a-63c8-a244-0f2a-361e5097764f@oss.nttdata.com>
 <CAPmGK15FuPVGx3TGHKShsbPKKtF1y58-ZLcKoxfN-nqLj1dZ=g@mail.gmail.com>
 <d888307d-c5b2-88e7-e2c0-f2228ad0756b@oss.nttdata.com>
 <CAPmGK158hrd=ZfXmgkmNFHivgh18e4oE2Gz151C2Q4OBDjZ08A@mail.gmail.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <CAPmGK158hrd=ZfXmgkmNFHivgh18e4oE2Gz151C2Q4OBDjZ08A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/3e57ebd7-b536-333e-c877-49a3ebb57079%40oss.nttdata.com>
Precedence: bulk


On 2022/02/07 14:35, Etsuro Fujita wrote:
>> 0001 patch failed to be applied. Could you rebase the patch?
> 
> Done.  Attached is an updated version of the patch set.

Thanks for updating the patch! Here are the review comments for 0001 patch.

I got the following compiler warning.

[16:58:07.120] connection.c: In function ‘pgfdw_finish_pre_commit_cleanup’:
[16:58:07.120] connection.c:1726:4: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
[16:58:07.120]  1726 |    PGresult   *res;
[16:58:07.120]       |    ^~~~~~~~


+			/* Ignore errors in the DEALLOCATE (see note above) */
+			if ((res = PQgetResult(entry->conn)) != NULL)

Doesn't PQgetResult() need to be called repeatedly until it returns NULL or the connection is lost because there can be more than one messages to receive?


+	if (pending_deallocs)
+	{
+		foreach(lc, pending_deallocs)

If pending_deallocs is NIL, we don't enter this foreach loop. So probably "if (pending_deallocs)" seems not necessary.


  			entry->keep_connections = defGetBoolean(def);
+		if (strcmp(def->defname, "parallel_commit") == 0)
+			entry->parallel_commit = defGetBoolean(def);

Isn't it better to use "else if" here, instead?


+static void do_sql_command_begin(PGconn *conn, const char *sql);
+static void do_sql_command_end(PGconn *conn, const char *sql);

To simplify the code more, I'm tempted to change do_sql_command() so that it just calls the above two functions, instead of calling PQsendQuery() and pgfw_get_result() directly. Thought? If we do this, probably we also need to change do_sql_command_end() so that it accepts boolean flag which specifies whether PQconsumeInput() is called or not, as follows.

     do_sql_command_end(PGconn *conn, const char *sql, bool consumeInput)
     {
         /*
	* If any data is expected to be available from the socket, consume it.
	* ...
	* When parallel_commit is enabled, since there can be a time window between
	* sending query and receiving result, we can expect data is already available
	* from the socket. In this case we try to consume it at first.... Otherwise..
	*/
         if (consumeInput && !PQconsumeInput(conn))
	    ...

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION