MIME-Version: 1.0
References: 
 <OS0PR01MB5716730C06159452335D870D947F9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1Lu-6oXMk7ZaGYLwm3CRLBuzueGbasyHnNpJxu6Mq3mmg@mail.gmail.com>
 <OS3PR01MB6275F145878B4A44586C46CE9E499@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <CAA4eK1LMVdS6uM7Tw7ANL0BetAd76TKkmAXNNQa0haTe2tax6g@mail.gmail.com>
 <OS3PR01MB6275EFC4B707650DAB9392859E4D9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <CAHut+PuVjRgGr4saN7qwq0oB8DANHVR7UfDiciB1Q3cYN54F6A@mail.gmail.com>
 <OS3PR01MB627567CF4B96A23DAFAF77269E4E9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <CAA4eK1KEf6w1azJytYnLhd2Lo-=rPkRh1Jv2ePcvs=SKqNQnAQ@mail.gmail.com>
In-Reply-To: 
 <CAA4eK1KEf6w1azJytYnLhd2Lo-=rPkRh1Jv2ePcvs=SKqNQnAQ@mail.gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Sat, 24 Sep 2022 17:10:06 +0530
Message-ID: 
 <CAA4eK1KjGNA8T8O77rRhkv6bRT6OsdQaEy--2hNrJFCc80bN0A@mail.gmail.com>
Subject: Re: Perform streaming logical transactions by background workers and
 parallel apply
To: "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>
Cc: Peter Smith <smithpb2250@gmail.com>,
	"houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>,
 Dilip Kumar <dilipbalaut@gmail.com>,
	Masahiko Sawada <sawada.mshk@gmail.com>,
 "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/mixed; boundary="00000000000025d93d05e96ac202"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1KjGNA8T8O77rRhkv6bRT6OsdQaEy--2hNrJFCc80bN0A%40mail.gmail.com>
Precedence: bulk

--00000000000025d93d05e96ac202
Content-Type: text/plain; charset="UTF-8"

On Thu, Sep 22, 2022 at 3:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Sep 22, 2022 at 8:59 AM wangw.fnst@fujitsu.com
> <wangw.fnst@fujitsu.com> wrote:
> >
>
> Few comments on v33-0001
> =======================
>

Some more comments on v33-0001
=============================
1.
+ /* Information from the corresponding LogicalRepWorker slot. */
+ uint16 logicalrep_worker_generation;
+
+ int logicalrep_worker_slot_no;
+} ParallelApplyWorkerShared;

Both these variables are read/changed by leader/parallel workers
without using any lock (mutex). It seems currently there is no problem
because of the way the patch is using in_parallel_apply_xact but I
think it won't be a good idea to rely on it. I suggest using mutex to
operate on these variables and also check if the slot_no is in a valid
range after reading it in parallel_apply_free_worker, otherwise error
out using elog.

2.
 static void
 apply_handle_stream_stop(StringInfo s)
 {
- if (!in_streamed_transaction)
+ ParallelApplyWorkerInfo *winfo = NULL;
+ TransApplyAction apply_action;
+
+ if (!am_parallel_apply_worker() &&
+ (!in_streamed_transaction && !stream_apply_worker))
  ereport(ERROR,
  (errcode(ERRCODE_PROTOCOL_VIOLATION),
  errmsg_internal("STREAM STOP message without STREAM START")));

This check won't be able to detect missing stream start messages for
parallel apply workers apart from the first pair of start/stop. I
thought of adding in_remote_transaction check along with
am_parallel_apply_worker() to detect the same but that also won't work
because the parallel worker doesn't reset it at the stop message.
Another possibility is to introduce yet another variable for this but
that doesn't seem worth it. I would like to keep this check simple.
Can you think of any better way?

3. I think we can skip sending start/stop messages from the leader to
the parallel worker because unlike apply worker it will process only
one transaction-at-a-time. However, it is not clear whether that is
worth the effort because it is sent after logical_decoding_work_mem
changes. For now, I have added a comment for this in the attached
patch but let me if I am missing something or if I am wrong.

4.
postgres=# select pid, leader_pid, application_name, backend_type from
pg_stat_activity;
  pid  | leader_pid | application_name |         backend_type
-------+------------+------------------+------------------------------
 27624 |            |                  | logical replication launcher
 17336 |            | psql             | client backend
 26312 |            |                  | logical replication worker
 26376 |            | psql             | client backend
 14004 |            |                  | logical replication worker

Here, the second worker entry is for the parallel worker. Isn't it
better if we distinguish this by keeping type as a logical replication
parallel worker? I think for this you need to change bgw_type in
logicalrep_worker_launch().

5. Can we name parallel_apply_subxact_info_add() as
parallel_apply_start_subtrans()?

Apart from the above, I have added/edited a few comments and made a
few other cosmetic changes in the attached.

-- 
With Regards,
Amit Kapila.

--00000000000025d93d05e96ac202
Content-Type: application/octet-stream; name="changes_atop_v33_1_amit.patch"
Content-Disposition: attachment; filename="changes_atop_v33_1_amit.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_l8fu8y1c0>
X-Attachment-Id: f_l8fu8y1c0

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvYXBwbHlwYXJhbGxl
bHdvcmtlci5jIGIvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9hcHBseXBhcmFsbGVs
d29ya2VyLmMKaW5kZXggNGYzYzRlZmVmZi4uZTFkM2RjOTljYSAxMDA2NDQKLS0tIGEvc3JjL2Jh
Y2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9hcHBseXBhcmFsbGVsd29ya2VyLmMKKysrIGIvc3Jj
L2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9hcHBseXBhcmFsbGVsd29ya2VyLmMKQEAgLTcx
Niw4ICs3MTYsOCBAQCBIYW5kbGVQYXJhbGxlbEFwcGx5TWVzc2FnZXModm9pZCkKIC8qCiAgKiBT
ZXQgdXAgYSBkeW5hbWljIHNoYXJlZCBtZW1vcnkgc2VnbWVudC4KICAqCi0gKiBXZSBzZXQgdXAg
YSBjb250cm9sIHJlZ2lvbiB0aGF0IGNvbnRhaW5zIGEgUGFyYWxsZWxBcHBseVdvcmtlclNoYXJl
ZCwgcGx1cwotICogb25lIHJlZ2lvbiB0byBlYWNoIG9mIG1lc3NhZ2UgcXVldWUgYW5kIGVycm9y
IHF1ZXVlLgorICogV2Ugc2V0IHVwIGEgY29udHJvbCByZWdpb24gdGhhdCBjb250YWlucyBhIGZp
eGVkIHdvcmtlciBpbmZvCisgKiAoUGFyYWxsZWxBcHBseVdvcmtlclNoYXJlZCksIGEgbWVzc2Fn
ZSBxdWV1ZSwgYW5kIGFuIGVycm9yIHF1ZXVlLgogICoKICAqIFJldHVybnMgdHJ1ZSBvbiBzdWNj
ZXNzLCBmYWxzZSBvbiBmYWlsdXJlLgogICovCkBAIC05MTYsOSArOTE2LDkgQEAgcGFyYWxsZWxf
YXBwbHlfZ2V0X2luX3hhY3QoUGFyYWxsZWxBcHBseVdvcmtlclNoYXJlZCAqd3NoYXJlZCkKICAq
IERlZmluZSBhIHNhdmVwb2ludCBmb3IgYSBzdWJ4YWN0IGluIHBhcmFsbGVsIGFwcGx5IHdvcmtl
ciBpZiBuZWVkZWQuCiAgKgogICogVGhlIHBhcmFsbGVsIGFwcGx5IHdvcmtlciBjYW4gZmlndXJl
IG91dCBpZiBhIG5ldyBzdWJ0cmFuc2FjdGlvbiB3YXMKLSAqIHN0YXJ0ZWQgYnkgY2hlY2tpbmcg
aWYgdGhlIG5ldyBjaGFuZ2UgYXJyaXZlZCB3aXRoIGRpZmZlcmVudCB4aWQuIEluIHRoYXQKKyAq
IHN0YXJ0ZWQgYnkgY2hlY2tpbmcgaWYgdGhlIG5ldyBjaGFuZ2UgYXJyaXZlZCB3aXRoIGEgZGlm
ZmVyZW50IHhpZC4gSW4gdGhhdAogICogY2FzZSBkZWZpbmUgYSBuYW1lZCBzYXZlcG9pbnQsIHNv
IHRoYXQgd2UgYXJlIGFibGUgdG8gcm9sbGJhY2sgdG8gaXQKLSAqIHNlcGFyYXRlbHkgbGF0ZXIu
CisgKiBpZiByZXF1aXJlZC4KICAqLwogdm9pZAogcGFyYWxsZWxfYXBwbHlfc3VieGFjdF9pbmZv
X2FkZChUcmFuc2FjdGlvbklkIGN1cnJlbnRfeGlkLCBUcmFuc2FjdGlvbklkIHRvcF94aWQpCmRp
ZmYgLS1naXQgYS9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9sb2dpY2FsL3dvcmtlci5jIGIvc3Jj
L2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC93b3JrZXIuYwppbmRleCBkMzRkZTdmNWFlLi42
N2FjMmJlODE4IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9sb2dpY2FsL3dv
cmtlci5jCisrKyBiL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvd29ya2VyLmMKQEAg
LTEzNDYsNiArMTM0NiwxMSBAQCBhcHBseV9oYW5kbGVfb3JpZ2luKFN0cmluZ0luZm8gcykKIAog
LyoKICAqIEhhbmRsZSBTVFJFQU0gU1RBUlQgbWVzc2FnZS4KKyAqCisgKiBYWFggV2UgY2FuIGF2
b2lkIHNlbmRpbmcgcGFpciBvZiB0aGUgU1RBUlQvU1RPUCBtZXNzYWdlcyB0byB0aGUgcGFyYWxs
ZWwKKyAqIHdvcmtlciBiZWNhdXNlIHVubGlrZSBhcHBseSB3b3JrZXIgaXQgd2lsbCBwcm9jZXNz
IG9ubHkgb25lCisgKiB0cmFuc2FjdGlvbi1hdC1hLXRpbWUuIEhvd2V2ZXIsIGl0IGlzIG5vdCBj
bGVhciB3aGV0aGVyIHRoYXQgaXMgd29ydGggdGhlCisgKiBlZmZvcnQgYmVjYXVzZSBpdCBpcyBz
ZW50IGFmdGVyIGxvZ2ljYWxfZGVjb2Rpbmdfd29ya19tZW0gY2hhbmdlcy4KICAqLwogc3RhdGlj
IHZvaWQKIGFwcGx5X2hhbmRsZV9zdHJlYW1fc3RhcnQoU3RyaW5nSW5mbyBzKQpAQCAtMTQ0Miw3
ICsxNDQ3LDcgQEAgYXBwbHlfaGFuZGxlX3N0cmVhbV9zdGFydChTdHJpbmdJbmZvIHMpCiAJCWNh
c2UgVFJBTlNfUEFSQUxMRUxfQVBQTFk6CiAKIAkJCS8qCi0JCQkgKiBNYWtlIHN1cmUgdGhlIGhh
bmRsZSBhcHBseV9kaXNwYXRjaCBtZXRob2RzIGFyZSBhd2FyZSB3ZSdyZSBpbgorCQkJICogTWFr
ZSBzdXJlIHRoZSBhcHBseV9kaXNwYXRjaCBtZXRob2RzIGFyZSBhd2FyZSB0aGF0IHdlJ3JlIGlu
CiAJCQkgKiBhIHJlbW90ZSB0cmFuc2FjdGlvbi4KIAkJCSAqLwogCQkJaW5fcmVtb3RlX3RyYW5z
YWN0aW9uID0gdHJ1ZTsKQEAgLTQ1NTYsMTYgKzQ1NjEsMjkgQEAgZ2V0X3RyYW5zYWN0aW9uX2Fw
cGx5X2FjdGlvbihUcmFuc2FjdGlvbklkIHhpZCwgUGFyYWxsZWxBcHBseVdvcmtlckluZm8gKip3
aW5mbykKIAkqd2luZm8gPSBOVUxMOwogCiAJaWYgKGFtX3BhcmFsbGVsX2FwcGx5X3dvcmtlcigp
KQorCXsKIAkJcmV0dXJuIFRSQU5TX1BBUkFMTEVMX0FQUExZOworCX0KIAllbHNlIGlmIChpbl9y
ZW1vdGVfdHJhbnNhY3Rpb24pCisJeworCQkvKgorCQkgKiBUaGUgaW5fcmVtb3RlX3RyYW5zYWN0
aW9uIGNhbiBiZSBzZXQgYm90aCBmb3IgbGVhZGVyIGFuZCBwYXJhbGxlbAorCQkgKiBhcHBseSB3
b3JrZXIgYnV0IGJlY2F1c2Ugb2YgYSBwcmlvciBjaGVjayBmb3IgdGhlIHBhcmFsbGVsIHdvcmtl
ciwKKwkJICogd2Ugd2lsbCByZWFjaCBoZXJlIG9ubHkgZm9yIGxlYWRlciBhcHBseSB3b3JrZXIu
CisJCSAqLwogCQlyZXR1cm4gVFJBTlNfTEVBREVSX0FQUExZOworCX0KIAogCS8qCiAJICogQ2hl
Y2sgaWYgd2UgYXJlIHByb2Nlc3NpbmcgdGhpcyB0cmFuc2FjdGlvbiB1c2luZyBhIHBhcmFsbGVs
IGFwcGx5CiAJICogd29ya2VyIGFuZCBpZiBzbywgc2VuZCB0aGUgY2hhbmdlcyB0byB0aGF0IHdv
cmtlci4KIAkgKi8KIAllbHNlIGlmICgoKndpbmZvID0gcGFyYWxsZWxfYXBwbHlfZmluZF93b3Jr
ZXIoeGlkKSkpCisJewogCQlyZXR1cm4gVFJBTlNfTEVBREVSX1NFTkRfVE9fUEFSQUxMRUw7CisJ
fQogCWVsc2UKKwl7CiAJCXJldHVybiBUUkFOU19MRUFERVJfU0VSSUFMSVpFOworCX0KIH0K
--00000000000025d93d05e96ac202--