Re: postgres_fdw: commit remote (sub)transactions in parallel during pre-commit

On Tue, Feb 8, 2022 at 3:49 AM Fujii Masao <masao.fujii@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