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

From: Etsuro Fujita <[email-masked]>
Date: Sun, 31 Oct 2021 18:05:44 +0900

Hi,

As I said before [1], I’m working on $SUBJECT.  Attached is a WIP
patch for that.  The patch is pretty simple: if a server option added
by the patch “parallel_commit” is enabled, 1) asynchronously send
COMMIT TRANSACTION (RELEASE SAVEPOINT) to all remote servers involved
in a local (sub)transaction, then 2) wait for the results from the
remote servers in the order that the command was sent to the remote
servers, when called from pgfdw_xact_callback (pgfdw_subxact_callback)
during pre-commit.  The patch also parallelizes clearing prepared
statements the same way during pre-commit.  (The option is false by
default.)

I evaluated the effectiveness of the patch using a simple
multi-statement transaction:

BEGIN;
SAVEPOINT s;
INSERT INTO ft1 VALUES (10, 10);
INSERT INTO ft2 VALUES (20, 20);
RELEASE SAVEPOINT s;
COMMIT;

where ft1 and ft2 are foreign tables created on different foreign
servers hosted on different machines.  I ran the transaction five
times using the patch with the option enabled/disabled, and measured
the latencies for the RELEASE and COMMIT commands in each run.  The
average latencies for these commands over the five runs are:

* RELEASE
  parallel_commit=0: 0.385 ms
  parallel_commit=1: 0.221 ms

* COMMIT
  parallel_commit=0: 1.660 ms
  parallel_commit=1: 0.861 ms

With the option enabled, the average latencies for both commands are
reduced significantly!

I think we could extend this to abort cleanup of remote
(sub)transactions during post-abort.  Anyway, I think this is useful,
so I’ll add this to the upcoming commitfest.

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/CAPmGK177E6HPcCQB4-s%2Bm9AcCZDHCC2drZy%2BFKnnvEXw9kXoXQ%40mail.gmail.com

---

From: Fujii Masao <[email-masked]>
Date: Mon, 1 Nov 2021 15:22:50 +0900

On 2021/10/31 18:05, Etsuro Fujita wrote:
> patch for that.

Thanks for the patch!

> The patch is pretty simple: if a server option added
> by the patch “parallel_commit” is enabled,

Could you tell me why the parameter is necessary?
Can't we always enable the feature?

> With the option enabled, the average latencies for both commands are

Sounds great!

> so I’ll add this to the upcoming commitfest.

Thanks!

+	/* Consume whatever data is available from the socket */
+	if (!PQconsumeInput(conn))
+		pgfdw_report_error(ERROR, NULL, conn, false, sql);

Without the patch, PQconsumeInput() is not called before pgfdw_get_result()
But could you tell me why you added PQconsumeInput() there?

When ignore_errors argument is true, the error reported by
PQconsumeInput() should be ignored?

Regards,

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

---

From: David Zhang <[email-masked]>
Date: Mon, 1 Nov 2021 15:46:39 -0700

> With the option enabled, the average latencies for both commands are
Followed your instructions, I performed some basic tests to compare the 
performance between before and after. In my testing environment (two 
foreign servers on the same local machine), the performance varies, 
sometimes the time spent on RELEASE and COMMIT without patch are close 
to after patched, but seems it always perform better after patched. Then 
I ran a 1-millions tuples insert, 5 times average is something like below,

Before
     RELEASE 0.171 ms, COMMIT 1.861 ms

After
     RELEASE 0.147 ms, COMMIT 1.305 ms

Best regards,
-- 
David

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

---

From: Etsuro Fujita <[email-masked]>
Date: Sun, 7 Nov 2021 18:06:04 +0900

> Can't we always enable the feature?

I don’t think parallel commit would cause performance degradation,
even in the case when there is just a single remote (sub)transaction
to commit when called from pgfdw_xact_callback
(pgfdw_subxact_callback), so I think it might be OK to enable it by
default.  But my concern about doing so is the remote side: during
those functions, if there are a lot of (sub)transactions on a single
remote server that need to be committed, parallel commit would
increase the remote server’s load at (sub)transaction end than serial
commit, which is the existing implementation, as the requests to
commit those (sub)transactions are sent to the remote server at the
same time; which some users might want to avoid.

> > so I’ll add this to the upcoming commitfest.

I'll update the patch as such in the next version.

> But could you tell me why you added PQconsumeInput() there?

The reason is that there might be the result already before calling
pgfdw_get_result(), in which case PQconsumeInput() followed by
PQisBusy() would allow us to call PQgetResult() without doing
WaitLatchOrSocket(), which I think is rather expensive.

> PQconsumeInput() should be ignored?

I’m not sure about that, because the error might be caused by e.g.,
OOM in the local server, in which case I don’t think it is safe to
ignore it and continue the (sub)transaction-end processing.

Thanks for reviewing!

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Sun, 7 Nov 2021 18:13:36 +0900

>      RELEASE 0.147 ms, COMMIT 1.305 ms

Good to know!  Thanks for testing!

Best regards,
Etsuro Fujita

---

From: Fujii Masao <[email-masked]>
Date: Mon, 8 Nov 2021 13:13:25 +0900

> same time; which some users might want to avoid.

Thanks for explaining this! But probably I failed to get your point.
Sorry... Whichever parallel or serial commit approach, the number of
transactions to commit on the remote server is the same, isn't it?
For example, please imagine the case where a client requests
ten transactions per second to the local server. Each transaction
accesses to the foreign table, so which means that ten transaction
commit operations per second are requested to the remote server.
Unless I'm missing something, this number doesn't change whether
the foreign transaction is committed in parallel way or not.
Thought?

> I'll update the patch as such in the next version.

IMO it's better to implement and commit these features gradually
if possible. Which would simplify the patch and make it
easier-to-review. So I think that it's better to implement
this feature as a separate patch.

> WaitLatchOrSocket(), which I think is rather expensive.

Understood. It's helpful to add the comment about why PQconsumeInput()
is called there.

Also could you tell me how much expensive it is?

> ignore it and continue the (sub)transaction-end processing.

But the existing code ignores the error at all, doesn't it?
If it's unsafe to do that, probably we should fix that at first?

Regards,

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 11 Nov 2021 02:27:28 +0000

Dear Fujita-san,

I love your proposal because it will remove a bottleneck 
for PostgreSQL build-in sharding.

I read your patch briefly and I think basically it's good.
Currently I have only one comment.

In your patch, postgres_fdw sends a COMMIT command to all entries in the hash table
and waits for the result without a timeout from the first entry.
I think this specification is good because it's very simple,
but if a COMMIT for a particular foreign server could take some time,
I thought it might be more efficient to stop waiting for results and look at the next entry.
This is how it works. First, we define a function similar to pgfdw_get_result()
so that we can specify the timeout time as an argument to WaitLatchOrSocket().
Then change the function called by do_sql_command_end () to the new one,
and change the callback function to skip if the result has not yet arrived

How is it? Is it an unnecessary assumption that COMMIT takes time? Or is this the next step?
I will put a PoC if needed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 16 Nov 2021 18:55:45 +0900

> > I think it might be OK to enable it by

Sorry, my explanation was not enough, but I don’t think this is always
true.  Let me explain using an example:

create server loopback foreign data wrapper postgres_fdw options
(dbname 'postgres', parallel_commit 'true');
create user mapping for current_user server loopback;
create table t1 (a int, b int);
create table t2 (a int, b int);
create foreign table ft1 (a int, b int) server loopback options
(table_name 't1');
create foreign table ft2 (a int, b int) server loopback options
(table_name 't2');
create role view_owner superuser;
create user mapping for view_owner server loopback;
grant SELECT on ft1 to view_owner;
create view v1 as select * from ft1;
alter view v1 owner to view_owner;

begin;
insert into v1 values (10, 10);
insert into ft2 values (20, 20);
commit;

For this transaction, since the first insert is executed as the view
owner while the second insert is executed as the current user, we
create a connection to the foreign server for each of the users to
execute the inserts.  This leads to sending two commit commands to the
foreign server at the same time during pre-commit.

To avoid spike loads on a remote server induced by such a workload, I
think it’s a good idea to have a server option to control whether this
is enabled, but I might be too worried about that, so I want to hear
the opinions of people.

> this feature as a separate patch.

Ok, I'll create a patch for abort cleanup separately.

> Understood. It's helpful to add the comment about why PQconsumeInput()

Ok.

> Also could you tell me how much expensive it is?

IIUC I think the overheads of WaitLatchOrSocket() incurred by a series
of epoll system calls are much larger compared to the overheads of
PQconsumeInput() incurred by a recv system call in non-blocking mode
when no data is available.  I didn’t do testing, though.

Actually, we already use this optimization in libpqrcv_receive() for
the caller of that function to avoid doing WaitLatchOrSocket()?

> If it's unsafe to do that, probably we should fix that at first?

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.

Thanks!

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 17 Nov 2021 13:00:04 +0900

Kuroda-san,

> I read your patch briefly and I think basically it's good.

Great!  Thanks for reviewing!

> I will put a PoC if needed.

Hmm, I'm not sure the cost-effectiveness of this optimization is
really high, because if the timeout expired, it means that something
unusual would have happened, and that it would take a long time for
the COMMIT command to complete (or abort at worst).  So even if we
processed the rest of the entries while waiting for the command
result, we cannot reduce the total time very much.  Maybe I'm missing
something, though.

Best regards,
Etsuro Fujita

---

From: Fujii Masao <[email-masked]>
Date: Thu, 18 Nov 2021 13:09:53 +0900

On 2021/11/16 18:55, Etsuro Fujita wrote:
> is enabled,

I understand your point. But even if the option is disabled (i.e.,
commit command is sent to each foreign server in serial way),
multiple queries still can run on the server concurrently and
which may cause performance "spike". Other clients may open several
sessions to the server and issue queries at the same time. Other
sessions using postgres_fdw may send commit command at the same time.
If we want to avoid that "spike", probably we need to decrease
max_connections or use connection pooling, etc. So ISTM that it's
half-baked and not enough to provide the option that controls
whether postgres_fdw issues commit command in parallel or serial way.

> but I might be too worried about that, so I want to hear
> the opinions of people.

Yes.

> when no data is available.  I didn’t do testing, though.

Understood.

Regards,

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 18 Nov 2021 21:45:29 +0900

On Thu, Nov 18, 2021 at 1:09 PM Fujii Masao <[email-masked]> wrote:
> max_connections or use connection pooling, etc.

I think that what you are discussing here would be a related but
different issue, because the patch doesn't increase the number of
connections to the remote server that are needed for processing a
single transaction than before.

My concern about the patch is that in parallel-commit mode,
transactions like the above example might increase the remote server's
load at transaction end than before, while using the same number of
connections to the remote server as before, because multiple COMMIT
commands are sent to the remote server at the same time, not
sequentially as before.  The option could be used to avoid such a
spike load without changing any settings on the remote server if
necessary.  Also, the option could be added at no extra cost, so there
seems to me to be no reason to remove it.

Anyway, I'd like to hear the opinions of others.

Thanks!

Best regards,
Etsuro Fujita

---

From: Fujii Masao <[email-masked]>
Date: Fri, 3 Dec 2021 18:07:25 +0900

> good to abort the local transaction unless we really have to.

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.

Regards,

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 24 Mar 2022 13:34:58 +0900

On Sat, Mar 5, 2022 at 7:32 PM Etsuro Fujita <[email-masked]> wrote:
> Attached is an updated version.

In the 0002 patch I introduced a macro for building an abort command
in preparation for the parallel abort patch (0003), but I moved it to
0003.  Attached is a new patch set.  The new version of 0002 is just a
cleanup patch (see the commit message in 0002), and I think it's
committable, so I'm planning to commit it, if no objections.

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 25 Mar 2022 15:46:02 +0900

On Thu, Mar 24, 2022 at 1:34 PM Etsuro Fujita <[email-masked]> wrote:
> Attached is a new patch set.  The new version of 0002 is just a

Done.

Attached is the 0003 patch, which is the same as the one I sent yesterday.

Best regards,
Etsuro Fujita

---

From: David Zhang <[email-masked]>
Date: Wed, 4 May 2022 14:38:54 -0700

Thanks a lot for the patch update.

On 2022-05-02 1:25 a.m., Etsuro Fujita wrote:
> Hi,
>
> On Wed, Apr 20, 2022 at 4:55 AM David Zhang <[email-masked]> wrote:
>> I tried to apply the patch to master and plan to run some tests, but got
>> below errors due to other commits.
> I rebased the patch against HEAD.  Attached is an updated patch.

Applied the patch v8 to master branch today, and the `make check` is OK. 
I also repeated previous performance tests on three virtual Ubuntu 
18.04, and the performance improvement of parallel abort in 10 times 
average is more consistent.

before:

     abort.1 = 2.6344 ms
     abort.2 = 4.2799 ms

after:

     abort.1 = 1.4105 ms
     abort.2 = 2.2075 ms

>> +     * remote server in parallel at (sub)transaction end.
>>
>> Here, I think the comment above could potentially apply to multiple
>> remote server(s).
> I agree on that point, but I think it's correct to say "the remote
> server" here, because we determine this for the given remote server.
> Maybe I'm missing something, so could you elaborate on it?
Your understanding is correct. I was thinking `remote server(s)` would 
be easy for end user to understand but this is a comment in source code, 
so either way is fine for me.
>> Not sure if there is a way to avoid repeated comments? For example, the
>> same comment below appears in two places (line 231 and line 296).
>>
>> +    /*
>> +     * If requested, consume whatever data is available from the socket.
>> +     * (Note that if all data is available, this allows
>> +     * pgfdw_get_cleanup_result to call PQgetResult without forcing the
>> +     * overhead of WaitLatchOrSocket, which would be large compared to the
>> +     * overhead of PQconsumeInput.)
>> +     */
> IMO I think it's OK to have this in multiple places, because 1) IMO it
> wouldn't be that long, and 2) we already duplicated comments like this
> in the same file in v14 and earlier.  Here is such an example in
> pgfdw_xact_callback() and pgfdw_subxact_callback() in that file in
> those versions:
>
>                      /*
>                       * If a command has been submitted to the remote server by
>                       * using an asynchronous execution function, the command
>                       * might not have yet completed.  Check to see if a
>                       * command is still being processed by the remote server,
>                       * and if so, request cancellation of the command.
>                       */
>
> Thanks for reviewing!  Sorry for the delay.
>
> Best regards,
> Etsuro Fujita
-- 
Best regards,
David

Software Engineer

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 6 May 2022 19:08:01 +0900

> so either way is fine for me.

Ok, but I noticed that the comment failed to mention that the
parallel_commit option is disabled by default.  Also, I noticed a
comment above it:

     * It's enough to determine this only when making new connection because
     * all the connections to the foreign server whose keep_connections option
     * is changed will be closed and re-made later.

This would apply to the parallel_commit option as well.  How about
updating these like the attached?  (I simplified the latter comment
and moved it to a more appropriate place.)

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 11 May 2022 19:39:07 +0900

> I noticed that the comment failed to mention that the

I’m planning to commit this as a follow-up patch for commit 04e706d42.

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 12 May 2022 17:46:27 +0900

Done.

Best regards,
Etsuro Fujita

---

From: Jacob Champion <[email-masked]>
Date: Thu, 30 Jun 2022 11:50:41 -0700

>> I’m planning to commit this as a follow-up patch for commit 04e706d42.

FYI, I think cfbot is confused about the patch under review here. (When
I first opened the thread I thought the patch had already been committed.)

For new reviewers: it looks like v8, upthread, is the proposal.

--Jacob

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 7 Jul 2022 17:22:32 +0900

Hi Jacob,

> I first opened the thread I thought the patch had already been committed.)

I should have attached the patch in the previous email.

> For new reviewers: it looks like v8, upthread, is the proposal.

The patch needs rebase due to commits 4036bcbbb, 8c8d307f8 and
82699edbf, so I updated the patch.  Attached is a new version, in
which I also tweaked comments a little bit.

Thanks for taking care of this!

Best regards,
Etsuro Fujita

---

From: David Zhang <[email-masked]>
Date: Fri, 30 Sep 2022 13:54:17 -0700

Hi Etsuro,

> which I also tweaked comments a little bit.

After rebase the file `postgres_fdw.out` and applied to master branch, 
make and make check are all ok for postgres_fdw.

-- 
David

Software Engineer

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 1 Nov 2022 19:24:36 +0900

Hi David,

> make and make check are all ok for postgres_fdw.

Thanks for testing!  Attached is a rebased version of the patch.

Best regards,
Etsuro Fujita