On Tue, Feb 8, 2022 at 3:49 AM Fujii Masao <masao.fu...@oss.nttdata.com> wrote: > 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] | ^~~~~~~~
Sorry, I didn’t notice this, because my compiler doesn’t produce it.
I tried to fix it. Attached is an updated version of the patch set.
I hope this works for you.
> + /* 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?
Yeah, we would receive a single message here, but PQgetResult must be
called repeatedly until it returns NULL (see the documentation note
about it in libpq.sgml); else the PQtransactionStatus of the
connection would remain PQTRANS_ACTIVE, causing the connection to be
closed at transaction end, because we do this in
pgfdw_reset_xact_state called from pgfdw_xact_callback:
/*
* If the connection isn't in a good idle state, it is marked as
* invalid or keep_connections option of its server is disabled, then
* discard it to recover. Next GetConnection will open a new
* connection.
*/
if (PQstatus(entry->conn) != CONNECTION_OK ||
PQtransactionStatus(entry->conn) != PQTRANS_IDLE ||
entry->changing_xact_state ||
entry->invalidated ||
!entry->keep_connections)
{
elog(DEBUG3, "discarding connection %p", entry->conn);
disconnect_pg_server(entry);
}
But I noticed a brown-paper-bag bug in the bit you showed above: the
if test should be modified as a while loop. :-( I fixed this in the
attached.
> + 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.
Yeah, I think we could omit the if test, but I added it to match other
places (see e.g., foreign_grouping_ok() in postgres_fdw.c). It looks
cleaner to me to have it before the loop.
> 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?
Yeah, that would be better. Done.
> +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.
Done. Actually, I was planning to do this for consistency with a
similar refactoring for pgfdw_cancel_query and
pgfdw_exec_cleanup_query that had been done
in the parallel-abort patch.
I tweaked comments/docs a little bit as well.
Thanks for reviewing!
Best regards,
Etsuro Fujita
v4-0001-postgres-fdw-Add-support-for-parallel-commit.patch
Description: Binary data
v4-0002-postgres_fdw-Minor-cleanup-for-pgfdw_abort_cleanup.patch
Description: Binary data
v4-0003-postgres-fdw-Add-support-for-parallel-abort.patch
Description: Binary data
