Subject: Re: Perform streaming logical transactions by background workers and parallel apply

From: Amit Kapila <[email-masked]>
Date: Mon, 2 Jan 2023 16:23:49 +0530

On Fri, Dec 30, 2022 at 3:55 PM [email-masked]
<[email-masked]> wrote:
>
> I've checked it and it looks good to me.
> Rebased the other patches and ran the pgident for the patch set.
>
> Attach the new patch set.
>

I have added a few DEBUG messages and changed a few comments in the
0001 patch. With that v71-0001* looks good to me and I'll commit it
later this week (by Thursday or Friday) unless there are any major
comments or objections.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 3 Jan 2023 05:40:22 +0000

> later this week (by Thursday or Friday) unless there are any major

Thanks for your improvement.

Rebased the patch set because the new change in HEAD (c8e1ba7).
Attach the new patch set.

Regards,
Wang wei

---

From: shveta malik <[email-masked]>
Date: Tue, 3 Jan 2023 14:39:08 +0530

> Attach the new patch set.

Hi,
In continuation with [1] and [2], I did some performance testing on
v70-0001 patch.

This test used synchronous logical replication and compared SQL
execution times before and after applying the patch.

The following cases are tested by varying logical_decoding_work_mem:
a) Bulk insert.
b) Bulk delete
c) Bulk update
b) Rollback to savepoint. (different percentages of changes in the
transaction are rolled back).

The tests are performed ten times, and the average of the middle eight is taken.

The scripts are the same as before [1]. The scripts for additional
update and delete testing are attached.

The results are as follows:

RESULT - bulk insert (5kk)
---------------------------------------------------------------
logical_decoding_work_mem     64kB        256kB       64MB
HEAD                                         34.475      34.222      34.400
patched                                      20.168      20.181      20.510
Compare with HEAD                -41.49%     -41.029%    -40.377%

RESULT - bulk delete (5kk)
---------------------------------------------------------------
logical_decoding_work_mem     64kB        256kB       64MB
HEAD                                         40.286      41.312      41.312
patched                                      23.749      23.759      23.480
Compare with HEAD                 -41.04%     -42.48%    -43.16%

RESULT - bulk update (5kk)
---------------------------------------------------------------
logical_decoding_work_mem     64kB        256kB       64MB
HEAD                                         63.650      65.260      65.459
patched                                      46.692      46.275      48.281
Compare with HEAD                -26.64%     -29.09%    -26.24%

RESULT - rollback 10% (5kk)
---------------------------------------------------------------
logical_decoding_work_mem     64kB        256kB       64MB
HEAD                                        33.386      33.213      31.990
patched                                      20.540      19.295      18.139
Compare with HEAD                 -38.47%     -41.90%    -43.29%

RESULT - rollback 20% (5kk)
---------------------------------------------------------------
logical_decoding_work_mem     64kB        256kB       64MB
HEAD                                         32.150      31.871      30.825
patched                                      19.331      19.366      18.285
Compare with HEAD                -39.87%     -39.23%     -40.68%

RESULT - rollback 30% (5kk)
---------------------------------------------------------------
logical_decoding_work_mem   64kB        256kB       64MB
HEAD                                      28.611      30.139      29.433
patched                                   19.632      19.838      18.374
Compare with HEAD               -31.38%     -34.17%      -37.57%

RESULT - rollback 50% (5kk)
---------------------------------------------------------------
logical_decoding_work_mem   64kB        256kB       64MB
HEAD                                       27.410      27.167     25.990
patched                                    19.982      18.749     18.048
Compare with HEAD               -27.099%    -30.98%      -30.55%

(if "Compare with HEAD" is a positive number, it means worse than
HEAD; if it is a negative number, it means better than HEAD.)

Summary:
Update shows 26-29% improvement, while insert and delete shows ~40% improvement.
In the case of rollback, the improvement is somewhat between 27-42%.
The improvement slightly decreases with larger amounts of data being
rolled back.

[1] https://www.postgresql.org/message-id/OSZPR01MB63103AA97349BBB858E27DEAFD499%40OSZPR01MB6310.jpnprd01.prod.outlook.com
[2] https://www.postgresql.org/message-id/OSZPR01MB6310174063C9144D2081F657FDE09%40OSZPR01MB6310.jpnprd01.prod.outlook.com

thanks
Shveta

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 4 Jan 2023 14:31:25 +0900

> Attach the new patch set.

There are some unused parameters in v72 patches:

+static bool
+pa_can_start(TransactionId xid)
+{
+        Assert(TransactionIdIsValid(xid));

'xid' is used only for the assertion check but I don't think it's necessary.

---
+/*
+ * Make sure the leader apply worker tries to read from our error
queue one more
+ * time. This guards against the case where we exit uncleanly without sending
+ * an ErrorResponse, for example because some code calls proc_exit directly.
+ */
+static void
+pa_shutdown(int code, Datum arg)

Similarly, we don't use 'code' here.

---
+/*
+ * Handle a single protocol message received from a single parallel apply
+ * worker.
+ */
+static void
+HandleParallelApplyMessage(ParallelApplyWorkerInfo *winfo, StringInfo msg)

In addition, the same is true for 'winfo'.

The rest looks good to me.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 4 Jan 2023 15:39:54 +0900

> Similarly, we don't use 'code' here.

This is necessary. Sorry for the noise.

Regards,

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 4 Jan 2023 10:55:34 +0000

On Wed, Jan 4, 2023 at 13:31 PM Masahiko Sawada <[email-masked]> wrote:
> > On Mon, Jan 2, 2023 at 18:54 PM Amit Kapila 
> > <[email-masked]>
> wrote:
> > > 0001 patch. With that v71-0001* looks good to me and I'll commit 
> > > it later this week (by Thursday or Friday) unless there are any 
> > > major comments or objections.
> >

Thanks for your comments!

> 'xid' is used only for the assertion check but I don't think it's necessary.

Agree. Removed this check.

> ---
> +/*
> + * Handle a single protocol message received from a single parallel 
> +apply
> + * worker.
> + */
> +static void
> +HandleParallelApplyMessage(ParallelApplyWorkerInfo *winfo, StringInfo 
> +msg)
> 

Agree. Removed this parameter.

Attach the new patch set.
Apart from addressing Sawada-San's comments, I also did some other minor
changes in the patch:

* Adjusted a testcase about crash restart in 023_twophase_stream.pl, I
  skipped the check for DEBUG msg as the msg might not be output if the crash happens
  before that.
* Adjusted the code in pg_lock_status() to make the fields of
  applytransaction lock display in more appropriate places.
* Add a comment to explain why we unlock the transaction before aborting the
  transaction in parallel apply worker.

Best regards,
Hou zj

---

From: Dilip Kumar <[email-masked]>
Date: Wed, 4 Jan 2023 16:52:37 +0530

On Wed, Jan 4, 2023 at 4:25 PM [email-masked]
<[email-masked]> wrote:

> Apart from addressing Sawada-San's comments, I also did some other minor

I have done a high-level review of 0001, and later I will do a
detailed review of this while reading through the patch I think some
of the comments need some changes..

1.
+ The deadlock can happen in
+ * the following ways:
+ *

+ * 4) Lock types
+ *
+ * Both the stream lock and the transaction lock mentioned above are
+ * session-level locks because both locks could be acquired outside the
+ * transaction, and the stream lock in the leader needs to persist across
+ * transaction boundaries i.e. until the end of the streaming transaction.

I think the Lock types should not be listed with the number 4).
Because point number 1,2 and 3 are explaining the way how deadlocks
can happen but 4) doesn't fall under that category.

2.
+ * Since the database structure (schema of subscription tables, constraints,
+ * etc.) of the publisher and subscriber could be different, applying
+ * transactions in parallel mode on the subscriber side can cause some
+ * deadlocks that do not occur on the publisher side.

I think this paragraph needs to be rephrased a bit.  It is saying that
some deadlock can occur on subscribers which did not occur on the
publisher.  I think what it should be conveying is that the deadlock
can occur due to concurrently applying the conflicting/dependent
transactions which are not conflicting/dependent on the publisher due
to <explain reason>.  Because if we create the same schema on the
publisher it might not have ended up in a deadlock instead it would
have been executed in sequence (due to lock waiting). So the main
point we are conveying is that the transaction which was independent
of each other on the publisher could be dependent on the subscriber
and they can end up in deadlock due to parallel apply.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com