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
morethan 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,
insteadof 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
