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
