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

From: Fujii Masao <[email-masked]>
Date: Thu, 13 Jan 2022 11:54:44 +0900

On 2022/01/06 17:29, Etsuro Fujita wrote:
> On Fri, Dec 3, 2021 at 6:07 PM Fujii Masao <[email-masked]> wrote:
>> On 2021/11/16 18:55, Etsuro Fujita wrote:
>>> I changed my mind; I’ll update the patch to ignore the error as
>>> before, because 1) as far as I know, there are no reports from the
>>> field concerning that we ignore all kinds of errors in cleaning up the
>>> prepared statements, so maybe we don’t need to change that, and 2) we
>>> already committed at least one of the remote transactions, so it’s not
>>> good to abort the local transaction unless we really have to.
> 
> Done.
> 
>> Are you planning to update the patch? In addition to this change,
>> at least documentation about new parallel_commit parameter needs
>> to be included in the patch.
> 
> Done.  Attached is a new version.
> 
> * 0001
> This is an updated version of the previous patch.  In addition to the
> above, I expanded a comment in do_sql_command_end() a bit to explain
> why we do PQconsumeInput() before doing pgfdw_get_result(), to address
> your comment.  Also, I moved the code to finish closing pending
> (sub)transactions in pgfdw_xact_callback()(pgfdw_subxact_callback())
> into separate functions.  Also, I modified regression test cases a bit
> to access multiple foreign servers.

Thanks for updating the patch!

At first I'm reading the 0001 patch. Here are the comments for the patch.

0001 patch failed to be applied. Could you rebase the patch?

+			entry->changing_xact_state = true;
+			do_sql_command_begin(entry->conn, "DEALLOCATE ALL");
+			pending_deallocs = lappend(pending_deallocs, entry);

Originally entry->changing_xact_state is not set to true when executing DEALLOCATE ALL. But the patch do that. Why do we need this change?

The source comment explains that we intentionally ignore errors in the DEALLOCATE. But the patch changes DEALLOCATE ALL so that it's executed via do_sql_command_begin() that can cause an error. Is this OK?

+		if (ignore_errors)
+			pgfdw_report_error(WARNING, res, conn, true, sql);

When DEALLOCATE fails, originally even warning message is not logged. But the patch changes DEALLOCATE so that its result is received via do_sql_command_end() that can log warning message even when ignore_errors argument is enabled. Why do we need to change the behavior?

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

Since parallel_commit is an option for foreign server, how the server with this option enabled is handled by postgres_fdw should be documented, instead?

+   <para>
+    Note that if many remote (sub)transactions are opened on a remote server
+    in a local (sub)transaction, this option might increase the remote
+    server’s load significantly when those remote (sub)transactions are
+    committed.  So be careful when using this option.
+   </para>

This paragraph should be inside the listitem for parallel_commit, shouldn't it?

async_capable=true also may cause the similar issue? If so, this kind of note should be documented also in async_capable?

This explains that the remote server's load will be increased *significantly*. But "significantly" part is really true? I'd like to know how much parallel_commit=true actually can increase the load in a remote server.

Regards,

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

---

From: Etsuro Fujita <[email-masked]>
Date: Mon, 7 Feb 2022 14:35:04 +0900

> At first I'm reading the 0001 patch. Here are the comments for the patch.

Thanks for reviewing!

> 0001 patch failed to be applied. Could you rebase the patch?

Done.  Attached is an updated version of the patch set.

> When DEALLOCATE fails, originally even warning message is not logged. But the patch changes DEALLOCATE so that its result is received via do_sql_command_end() that can log warning message even when…

Yeah, we don’t need to change the behavior as discussed before, so I
fixed these.  I worked on the patch after a while, so I forgot about
that.  :-(

> Since parallel_commit is an option for foreign server, how the server with this option enabled is handled by postgres_fdw should be documented, instead?

Agreed.  I rewrote this slightly like the attached.  Does that make sense?

> This paragraph should be inside the listitem for parallel_commit, shouldn't it?

I put this note outside, because it’s rewritten to a note about both
the parallel_commit and parallel_abort options in the following patch.
But it would be good to make the parallel-commit patch independent, so
I moved it into the list.

> async_capable=true also may cause the similar issue? If so, this kind of note should be documented also in async_capable?

That’s right.  I think it would be good to add a similar note about
that, but I’d like to leave that for another patch.

> This explains that the remote server's load will be increased *significantly*. But "significantly" part is really true?

I think that that would depend on how many transactions are committed
on the remote side at the same time.  But the word “significantly”
might be too strong, so I dropped the word.

> I'd like to know how much parallel_commit=true actually can increase the load in a remote server.

Ok, I’ll do a load test.

About the #0002 patch:
This is in preparation for the parallel-abort patch (#0003), but I’d
like to propose a minor cleanup for commit 85c696112: 1) build an
abort command to be sent to the remote in pgfdw_abort_cleanup(), using
a macro, only when/if necessary, as before, and 2) add/modify comments
a little bit.

Sorry for the delay again.

Best regards,
Etsuro Fujita

---

From: Fujii Masao <[email-masked]>
Date: Tue, 8 Feb 2022 03:49:47 +0900

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