MIME-Version: 1.0
References: 
 <CAA4eK1JQTDXTfvJ5d+L0ggG4+doyd0Xji=e0OJsb=qcn_jWALA@mail.gmail.com>
 <OS0PR01MB5716D6D2765E54DC739F288E940A9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB5716997A7115715F9E4EE520940D9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1JEFVZcymteMYXhYghiX7Lb=MKuEsfxgD1VfaRrLCHyzg@mail.gmail.com>
 <OS0PR01MB571663F65904D00895AD159994109@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1JuRPUY2Dx=vUToRksTmW0ptqMVT3K32g0368f_ZCk-zg@mail.gmail.com>
 <OS0PR01MB5716B802A1733548A99761AE94129@OS0PR01MB5716.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <OS0PR01MB5716B802A1733548A99761AE94129@OS0PR01MB5716.jpnprd01.prod.outlook.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Wed, 30 Nov 2022 16:23:50 +0530
Message-ID: 
 <CAA4eK1LGKYUDFZ_jFPrU497wQf2HNvt5a+tCTpqSeWSG6kfpSA@mail.gmail.com>
Subject: Re: Perform streaming logical transactions by background workers and
 parallel apply
To: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>,
	"wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>,
 Peter Smith <smithpb2250@gmail.com>,
	Dilip Kumar <dilipbalaut@gmail.com>,
 "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/mixed; boundary="0000000000001dcca705eeadecf3"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1LGKYUDFZ_jFPrU497wQf2HNvt5a%2BtCTpqSeWSG6kfpSA%40mail.gmail.com>
Precedence: bulk

--0000000000001dcca705eeadecf3
Content-Type: text/plain; charset="UTF-8"

On Tue, Nov 29, 2022 at 10:18 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> Attach the new version patch which addressed all comments.
>

Some comments on v53-0002*
========================
1. I think testing the scenario where the shm_mq buffer is full
between the leader and parallel apply worker would require a large
amount of data and then also there is no guarantee. How about having a
developer GUC [1] force_apply_serialize which allows us to serialize
the changes and only after commit the parallel apply worker would be
allowed to apply it?

I am not sure if we can reliably test the serialization of partial
changes (like some changes have been already sent to parallel apply
worker and then serialization happens) but at least we can test the
serialization of complete xacts and their execution via parallel apply
worker.

2.
+ /*
+ * The stream lock is released when processing changes in a
+ * streaming block, so the leader needs to acquire the lock here
+ * before entering PARTIAL_SERIALIZE mode to ensure that the
+ * parallel apply worker will wait for the leader to release the
+ * stream lock.
+ */
+ if (in_streamed_transaction &&
+ action != LOGICAL_REP_MSG_STREAM_STOP)
+ {
+ pa_lock_stream(winfo->shared->xid, AccessExclusiveLock);

This comment is not completely correct because we can even acquire the
lock for the very streaming chunk. This check will work but doesn't
appear future-proof or at least not very easy to understand though I
don't have a better suggestion at this stage. Can we think of a better
check here?

3. I have modified a few comments in v53-0002* patch and the
incremental patch for the same is attached.

[1] - https://www.postgresql.org/docs/devel/runtime-config-developer.html

-- 
With Regards,
Amit Kapila.

--0000000000001dcca705eeadecf3
Content-Type: application/octet-stream; name="changes_amit_v53_0002.patch"
Content-Disposition: attachment; filename="changes_amit_v53_0002.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_lb3j5ks30>
X-Attachment-Id: f_lb3j5ks30

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvYXBwbHlwYXJhbGxl
bHdvcmtlci5jIGIvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9hcHBseXBhcmFsbGVs
d29ya2VyLmMKaW5kZXggM2FjZjlkNWZiOS4uMWZjNjJjZGJhMiAxMDA2NDQKLS0tIGEvc3JjL2Jh
Y2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9hcHBseXBhcmFsbGVsd29ya2VyLmMKKysrIGIvc3Jj
L2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9hcHBseXBhcmFsbGVsd29ya2VyLmMKQEAgLTc0
LDEwICs3NCwxMCBAQAogICogbWVzc2FnZXMsIGFuZCB0aGlzIHdhaXQgZG9lc24ndCBhcHBlYXIg
aW4gbG1nci4KICAqCiAgKiBUbyByZXNvbHZlIHRoaXMgaXNzdWUsIHdlIHVzZSBub24tYmxvY2tp
bmcgd3JpdGUgYW5kIHdhaXQgd2l0aCBhIHRpbWVvdXQuIElmCi0gKiB0aW1lb3V0IGlzIGV4Y2Vl
ZGVkLCB0aGUgTEEgd2lsbCBzZXJpYWxpemUgdGhlIG1lc3NhZ2UgdG8gYSBmaWxlIGFuZAorICog
dGhlIHRpbWVvdXQgaXMgZXhjZWVkZWQsIHRoZSBMQSB3aWxsIHNlcmlhbGl6ZSB0aGUgbWVzc2Fn
ZSB0byBhIGZpbGUgYW5kCiAgKiBpbmRpY2F0ZSBQQS0yIHRoYXQgaXQgbmVlZHMgdG8gcmVhZCB0
aGF0IGZpbGUgZm9yIHRoZSByZW1haW5pbmcgbWVzc2FnZXMuCiAgKiBUaGVuIExBIHdpbGwgc3Rh
cnQgd2FpdGluZyBmb3IgY29tbWl0IHdoaWNoIHdpbGwgZGV0ZWN0IGRlYWRsb2NrIGlmIGFueS4K
LSAqIChTZWUgcGFfc2VuZF9kYXRhKCkgYW5kIHR5cGVkZWYgZW51bSBUcmFuc0FwcGx5QWN0aW9u
KQorICogKFNlZSBwYV9zZW5kX2RhdGEoKSBhbmQgZW51bSBUcmFuc0FwcGx5QWN0aW9uKQogICoK
ICAqIDQpIExvY2sgdHlwZQogICoKQEAgLTExNDcsOSArMTE0NywxMCBAQCBwYV9zZW5kX2RhdGEo
UGFyYWxsZWxBcHBseVdvcmtlckluZm8gKndpbmZvLCBTaXplIG5ieXRlcywgY29uc3Qgdm9pZCAq
ZGF0YSkKIAkJCUxvZ2ljYWxSZXBNc2dUeXBlIGFjdGlvbjsKIAogCQkJLyoKLQkJCSAqIFRoZSBw
YXJhbGxlbCBhcHBseSB3b3JrZXIgbWlnaHQgYmUgc3R1Y2sgZm9yIHNvbWUgcmVhc29uLCBzbwot
CQkJICogc3RvcCBzZW5kaW5nIGRhdGEgZGlyZWN0bHkgdG8gaXQgYW5kIHN0YXJ0IHRvIHNlcmlh
bGl6ZSBkYXRhIHRvCi0JCQkgKiBmaWxlcyBpbnN0ZWFkLgorCQkJICogVGhlIHBhcmFsbGVsIGFw
cGx5IHdvcmtlciBjb3VsZCBiZSBzdHVjayBmb3Igc29tZSByZWFzb24gKHNheQorCQkJICogd2Fp
dGluZyBvbiBzb21lIGxvY2sgYnkgb3RoZXIgYmFja2VuZCksIHNvIHN0b3AgdHJ5aW5nIHRvIHNl
bmQKKwkJCSAqIGRhdGEgZGlyZWN0bHkgdG8gaXQgYW5kIGluc3RlYWQgc3RhcnQgdG8gc2VyaWFs
aXplIGRhdGEgdG8KKwkJCSAqIGZpbGUgaW5zdGVhZC4KIAkJCSAqLwogCQkJd2luZm8tPnNlcmlh
bGl6ZV9jaGFuZ2VzID0gdHJ1ZTsKIApkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvcmVwbGljYXRp
b24vbG9naWNhbC93b3JrZXIuYyBiL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvd29y
a2VyLmMKaW5kZXggYjVmYjc0NTcyZS4uYzUxZDkyYjY3ZiAxMDA2NDQKLS0tIGEvc3JjL2JhY2tl
bmQvcmVwbGljYXRpb24vbG9naWNhbC93b3JrZXIuYworKysgYi9zcmMvYmFja2VuZC9yZXBsaWNh
dGlvbi9sb2dpY2FsL3dvcmtlci5jCkBAIC0xMzMxLDcgKzEzMzEsNiBAQCBhcHBseV9oYW5kbGVf
c3RyZWFtX3ByZXBhcmUoU3RyaW5nSW5mbyBzKQogCQkJICovCiAJCQlwYV91bmxvY2tfc3RyZWFt
KHdpbmZvLT5zaGFyZWQtPnhpZCwgQWNjZXNzRXhjbHVzaXZlTG9jayk7CiAKLQkJCS8qIFNlbmQg
U1RSRUFNIFBSRVBBUkUgbWVzc2FnZSB0byB0aGUgcGFyYWxsZWwgYXBwbHkgd29ya2VyLiAqLwog
CQkJaWYgKGFwcGx5X2FjdGlvbiA9PSBUUkFOU19MRUFERVJfU0VORF9UT19QQVJBTExFTCkKIAkJ
CQlwYV9zZW5kX2RhdGEod2luZm8sIHMtPmxlbiwgcy0+ZGF0YSk7CiAJCQllbHNlCkBAIC0xMzM5
LDYgKzEzMzgsMTAgQEAgYXBwbHlfaGFuZGxlX3N0cmVhbV9wcmVwYXJlKFN0cmluZ0luZm8gcykK
IAkJCQkJCQkJCQkJIExPR0lDQUxfUkVQX01TR19TVFJFQU1fUFJFUEFSRSwKIAkJCQkJCQkJCQkJ
ICZvcmlnaW5hbF9tc2cpOwogCisJCQkvKgorCQkJICogSXQgaXMgcG9zc2libGUgdGhhdCB3aGls
ZSBzZW5kaW5nIHRoaXMgY2hhbmdlIHRvIHBhcmFsbGVsIGFwcGx5CisJCQkgKiB3b3JrZXIgd2Ug
bmVlZCB0byBzd2l0Y2ggdG8gc2VyaWFsaXplIG1vZGUuCisJCQkgKi8KIAkJCWlmICh3aW5mby0+
c2VyaWFsaXplX2NoYW5nZXMpCiAJCQkJcGFfc2V0X2ZpbGVzZXRfc3RhdGUod2luZm8tPnNoYXJl
ZCwgRlNfUkVBRFkpOwogCkBAIC0xODQwLDYgKzE4NDMsMTAgQEAgYXBwbHlfaGFuZGxlX3N0cmVh
bV9hYm9ydChTdHJpbmdJbmZvIHMpCiAKIAkJCWlmICh0b3BsZXZlbF94YWN0KQogCQkJeworCQkJ
CS8qCisJCQkJICogSXQgaXMgcG9zc2libGUgdGhhdCB3aGlsZSBzZW5kaW5nIHRoaXMgY2hhbmdl
IHRvIHBhcmFsbGVsIGFwcGx5CisJCQkJICogd29ya2VyIHdlIG5lZWQgdG8gc3dpdGNoIHRvIHNl
cmlhbGl6ZSBtb2RlLgorCQkJCSAqLwogCQkJCWlmICh3aW5mby0+c2VyaWFsaXplX2NoYW5nZXMp
CiAJCQkJCXBhX3NldF9maWxlc2V0X3N0YXRlKHdpbmZvLT5zaGFyZWQsCiAJCQkJCQkJCQkJIEZT
X1JFQURZKTsKQEAgLTIwNjIsMTMgKzIwNjksMTYgQEAgYXBwbHlfaGFuZGxlX3N0cmVhbV9jb21t
aXQoU3RyaW5nSW5mbyBzKQogCQkJICovCiAJCQlwYV91bmxvY2tfc3RyZWFtKHhpZCwgQWNjZXNz
RXhjbHVzaXZlTG9jayk7CiAKLQkJCS8qIFNlbmQgU1RSRUFNIENPTU1JVCBtZXNzYWdlIHRvIHRo
ZSBwYXJhbGxlbCBhcHBseSB3b3JrZXIuICovCiAJCQlpZiAoYXBwbHlfYWN0aW9uID09IFRSQU5T
X0xFQURFUl9TRU5EX1RPX1BBUkFMTEVMKQogCQkJCXBhX3NlbmRfZGF0YSh3aW5mbywgcy0+bGVu
LCBzLT5kYXRhKTsKIAkJCWVsc2UKIAkJCQlzdHJlYW1fb3Blbl9hbmRfd3JpdGVfY2hhbmdlKHhp
ZCwgTE9HSUNBTF9SRVBfTVNHX1NUUkVBTV9DT01NSVQsCiAJCQkJCQkJCQkJCSAmb3JpZ2luYWxf
bXNnKTsKIAorCQkJLyoKKwkJCSAqIEl0IGlzIHBvc3NpYmxlIHRoYXQgd2hpbGUgc2VuZGluZyB0
aGlzIGNoYW5nZSB0byBwYXJhbGxlbCBhcHBseQorCQkJICogd29ya2VyIHdlIG5lZWQgdG8gc3dp
dGNoIHRvIHNlcmlhbGl6ZSBtb2RlLgorCQkJICovCiAJCQlpZiAod2luZm8tPnNlcmlhbGl6ZV9j
aGFuZ2VzKQogCQkJCXBhX3NldF9maWxlc2V0X3N0YXRlKHdpbmZvLT5zaGFyZWQsCiAJCQkJCQkJ
CQkgRlNfUkVBRFkpOwo=
--0000000000001dcca705eeadecf3--