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

From: Etsuro Fujita <[email-masked]>
Date: Fri, 11 Feb 2022 21:59:18 +0900

On Tue, Feb 8, 2022 at 3:49 AM Fujii Masao <[email-masked]> 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

---

From: Fujii Masao <[email-masked]>
Date: Fri, 18 Feb 2022 01:46:38 +0900

> I tweaked comments/docs a little bit as well.

Thanks for updating the patches!

I reviewed 0001 patch. It looks good to me except the following minor things. If these are addressed, I think that the 001 patch can be marked as ready for committer.

+	 * Also determine to commit (sub)transactions opened on the remote server
+	 * in parallel at (sub)transaction end.

Like the comment "Determine whether to keep the connection ...", "determine to commit" should be "determine whether to commit"?

"remote server" should be "remote servers"?

+	curlevel = GetCurrentTransactionNestLevel();
+	snprintf(sql, sizeof(sql), "RELEASE SAVEPOINT s%d", curlevel);

Why does pgfdw_finish_pre_subcommit_cleanup() need to call GetCurrentTransactionNestLevel() and construct the "RELEASE SAVEPOINT" query string again? pgfdw_subxact_callback() already does them and probably we can make it pass either of them to pgfdw_finish_pre_subcommit_cleanup() as its argument.

+       This option controls whether <filename>postgres_fdw</filename> commits
+       remote (sub)transactions opened on a foreign server in a local
+       (sub)transaction in parallel when the local (sub)transaction commits.

"a foreign server" should be "foreign servers"?

"in a local (sub)transaction" part seems not to be necessary.

Regards,

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

---

From: David Zhang <[email-masked]>
Date: Fri, 18 Feb 2022 13:55:19 -0800

Thanks a lot for updating the patch.

Tried to apply the patches to master branch, no warning found and 
regression test passed.

Now, we have many places (5) calling the same function with a constant 
number 30000. Is this a good time to consider redefine this number a 
macro somewhere?

Thank you,

On 2022-02-17 8:46 a.m., Fujii Masao wrote:
> I reviewed 0001 patch. It looks good to me except the following minor 
> things. If these are addressed, I think that the 001 patch can be 
> marked as ready for committer.
>
> Like the comment "Determine whether to keep the connection ...", 
> "determine to commit" should be "determine whether to commit"?
>
> Why does pgfdw_finish_pre_subcommit_cleanup() need to call 
> GetCurrentTransactionNestLevel() and construct the "RELEASE SAVEPOINT" 
> query string again? pgfdw_subxact_callback() already does them and 
> probably we can make it pass either of them to 
> pgfdw_finish_pre_subcommit_cleanup() as its argument.
>
>
-- 
David

Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca

---

From: Etsuro Fujita <[email-masked]>
Date: Mon, 21 Feb 2022 14:45:10 +0900

> I reviewed 0001 patch. It looks good to me except the following minor things. If these are addressed, I think that the 001 patch can be marked as ready for committer.

OK

> Like the comment "Determine whether to keep the connection ...", "determine to commit" should be "determine whether to commit"?

Agreed.  I’ll change it as such.

> "remote server" should be "remote servers"?

Maybe I’m missing something, but we determine this for the given
remote server, so it seems to me correct to say “the remote server”,
not “the remote servers“.

> Why does pgfdw_finish_pre_subcommit_cleanup() need to call GetCurrentTransactionNestLevel() and construct the "RELEASE SAVEPOINT" query string again? pgfdw_subxact_callback() already does them and…

Yeah, that would save cycles, but I think that that makes code a bit
unclean IMO.  (To save cycles, I think we could also modify
pgfdw_subxact_callback() to reuse the query in the while loop in that
function when processing multiple open remote subtransactions there,
but that would make code a bit complicated, so I don’t think it’s a
good idea to do so, either.)  So I’d vote for reconstructing the query
in pgfdw_finish_pre_subcommit_cleanup() as we do in
pgfdw_subxact_callback().

To avoid calling GetCurrentTransactionNestLevel() again, I think we
could pass the curlevel variable to that function.

> "a foreign server" should be "foreign servers"?

I thought it would be good to say “a foreign server”, not “foreign
servers”, because it makes clear that even remote transactions opened
on a single foreign server are committed in parallel.  (To say that
this option is not for a specific foreign server, I added to the
documentation “This option can only be specified for foreign
servers”.)

> "in a local (sub)transaction" part seems not to be necessary.

And I thought adding it would make clear which remote transactions are
committed in parallel.  But maybe I’m missing something, so could you
elaborate a bit more on these?

Thanks for reviewing!

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Mon, 21 Feb 2022 14:55:07 +0900

> Tried to apply the patches to master branch, no warning found and

Thanks for testing!

> number 30000. Is this a good time to consider redefine this number a

Yeah, I think that is a good idea.  I’ll do so in the next version of
the parallel-abort patch (#0003) if no objections.

Best regards,
Etsuro Fujita

---

From: Fujii Masao <[email-masked]>
Date: Tue, 22 Feb 2022 01:03:51 +0900

> Agreed.  I’ll change it as such.

Thanks! If that's updated, IMO it's ok to commit the 0001 patch.
After the commit, I will review 0002 and 0003 patches.

Regards,

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 23 Feb 2022 15:30:37 +0900

> > Agreed.  I’ll change it as such.

Done.

> Thanks! If that's updated, IMO it's ok to commit the 0001 patch.

Cool!  Attached is an updated patch.  Other changes other than that:
1) I added the curlevel parameter to
pgfdw_finish_pre_subcommit_cleanup() to avoid doing
GetCurrentTransactionNestLevel() there, as proposed, and 2) tweaked
comments a bit further, mostly for/in
pgfdw_finish_pre_commit_cleanup() and
pgfdw_finish_pre_subcommit_cleanup().  Barring objections, I’ll commit
the patch.

Thanks for reviewing!

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 24 Feb 2022 14:49:12 +0900

On Wed, Feb 23, 2022 at 3:30 PM Etsuro Fujita <[email-masked]> wrote:
> Attached is an updated patch.

> Barring objections, I’ll commit
> the patch.

I have committed the patch.  I think the 0003 patch needs rebase.
I'll update the patch.

Best regards,
Etsuro Fujita