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>
 <CAA4eK1LGKYUDFZ_jFPrU497wQf2HNvt5a+tCTpqSeWSG6kfpSA@mail.gmail.com>
 <CAA4eK1Jwh7j86Egk1cye=x2R_yrTjzXGj7Fx12wVybBAEq91kA@mail.gmail.com>
 <OS0PR01MB571690D6FF24E9D000ECBDBD94199@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB571639E4141EBF8A2C501E6994189@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1+ELh2vPre3JHyoeKV0A9_V7aXQD0QBPv86WEn7P_rK-g@mail.gmail.com>
 <OS0PR01MB57166A377638E57823219E32941A9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1KpN=dZPcCj=TLwh3wk9uvPB-M1Jrjw7wt+-L62YrDHFQ@mail.gmail.com>
 <OS0PR01MB571604D401DE7A0659BD6609941D9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1LJMpBYbgDVz=g4qig8C2bM10PK=DgW7o1zn8X426vJ+Q@mail.gmail.com>
 <OS0PR01MB571663B4EC5D8BE749AACD98941E9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAD21AoD_3ta_29sopuf_KJCExverP5808HjwDeqaHDrbqbxYww@mail.gmail.com>
 <OS0PR01MB5716D38323B07EB0A0D097F194E09@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1Kumnc+AR86AGJ666+4Ebfny+nMbs_JZSW3O__UEiGp6w@mail.gmail.com>
 <OS0PR01MB57169DAA9A2A6E68EE5E05F094E19@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAD21AoA-i_Gr1Quo2JN56ORafL=phqD=5hUvbA40EHMRZjArQA@mail.gmail.com>
 <OS0PR01MB5716E94C5E503CC53177A22994E69@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1KkrkAzO-=_k_6s5V1+uNUFkvxMK-33MaFw=JCw5Hqs3w@mail.gmail.com>
 <CAA4eK1LRqEXAmh2ZEG-8XK_xDLu2Zzdoj86v4iNXDy++B2SBnQ@mail.gmail.com>
 <OS0PR01MB5716C3561CF53027F22B47AF94E79@OS0PR01MB5716.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <OS0PR01MB5716C3561CF53027F22B47AF94E79@OS0PR01MB5716.jpnprd01.prod.outlook.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Mon, 19 Dec 2022 18:17:25 +0530
Message-ID: 
 <CAA4eK1KdVRffYEYiv9=4tVWhP=Jkfh1h6FM0A1R0NEe8cLT8+g@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="00000000000058201d05f02db9a6"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1KdVRffYEYiv9%3D4tVWhP%3DJkfh1h6FM0A1R0NEe8cLT8%2Bg%40mail.gmail.com>
Precedence: bulk

--00000000000058201d05f02db9a6
Content-Type: text/plain; charset="UTF-8"

On Sat, Dec 17, 2022 at 7:34 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> Agreed. I have addressed all the comments and did some cosmetic changes.
> Attach the new version patch set.
>

Few comments:
============
1.
+ if (fileset_state == FS_SERIALIZE_IN_PROGRESS)
+ {
+ pa_lock_stream(MyParallelShared->xid, AccessShareLock);
+ pa_unlock_stream(MyParallelShared->xid, AccessShareLock);
+ }
+
+ /*
+ * We cannot read the file immediately after the leader has serialized all
+ * changes to the file because there may still be messages in the memory
+ * queue. We will apply all spooled messages the next time we call this
+ * function, which should ensure that there are no messages left in the
+ * memory queue.
+ */
+ else if (fileset_state == FS_SERIALIZE_DONE)
+ {

Once we have waited in the FS_SERIALIZE_IN_PROGRESS, the file state
can be FS_SERIALIZE_DONE immediately after that. So, won't it be
better to have a separate if block for FS_SERIALIZE_DONE state? If you
agree to do so then we can probably remove the comment: "* XXX It is
possible that immediately after we have waited for a lock in ...".

2.
+void
+pa_decr_and_wait_stream_block(void)
+{
+ Assert(am_parallel_apply_worker());
+
+ if (pg_atomic_sub_fetch_u32(&MyParallelShared->pending_stream_count, 1) == 0)

I think here the count can go negative when we are in serialize mode
because we don't increase it for serialize mode. I can't see any
problem due to that but OTOH, this doesn't seem to be intended because
in the future if we decide to implement the functionality of switching
back to non-serialize mode, this could be a problem. Also, I guess we
don't even need to try locking/unlocking the stream lock in that case.
One idea to avoid this is to check if the pending count is zero then
if file_set in not available raise an error (elog ERROR), otherwise,
simply return from here.

3. In apply_handle_stream_stop(), we are setting backendstate as idle
for cases TRANS_LEADER_SEND_TO_PARALLEL and TRANS_PARALLEL_APPLY. For
other cases, it is set by stream_stop_internal. I think it would be
better to set the state explicitly for all cases to make the code look
consistent and remove it from stream_stop_internal(). The other reason
to remove setting the state from stream_stop_internal() is that when
that function is invoked from other places like
apply_handle_stream_commit(), it seems to be setting the idle before
actually we reach the idle state.

4. Apart from the above, I have made a few changes in the comments,
see attached.

-- 
With Regards,
Amit Kapila.

--00000000000058201d05f02db9a6
Content-Type: application/octet-stream; name="changes_amit_1.patch"
Content-Disposition: attachment; filename="changes_amit_1.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_lbuskkd20>
X-Attachment-Id: f_lbuskkd20

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvYXBwbHlwYXJhbGxl
bHdvcmtlci5jIGIvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9hcHBseXBhcmFsbGVs
d29ya2VyLmMKaW5kZXggNzQyMjUyMTYyNi4uN2EzMDgyOGQ0NCAxMDA2NDQKLS0tIGEvc3JjL2Jh
Y2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9hcHBseXBhcmFsbGVsd29ya2VyLmMKKysrIGIvc3Jj
L2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9hcHBseXBhcmFsbGVsd29ya2VyLmMKQEAgLTEz
OCwxMCArMTM4LDExIEBACiAgKiB3YWl0IHRvIHNlbmQgbWVzc2FnZXMsIGFuZCB0aGlzIHdhaXQg
ZG9lc24ndCBhcHBlYXIgaW4gbG1nci4KICAqCiAgKiBUbyBhdm9pZCB0aGlzIHdhaXQsIHdlIHVz
ZSBhIG5vbi1ibG9ja2luZyB3cml0ZSBhbmQgd2FpdCB3aXRoIGEgdGltZW91dC4gSWYKLSAqIHRo
ZSB0aW1lb3V0IGlzIGV4Y2VlZGVkLCB0aGUgTEEgd2lsbCBzZXJpYWxpemUgdGhlIG1lc3NhZ2Ug
dG8gYSBmaWxlIGFuZAotICogaW5kaWNhdGUgUEEtMiB0aGF0IGl0IG5lZWRzIHRvIHJlYWQgdGhh
dCBmaWxlIGZvciB0aGUgcmVtYWluaW5nIG1lc3NhZ2VzLgotICogVGhlbiBMQSB3aWxsIHN0YXJ0
IHdhaXRpbmcgZm9yIGNvbW1pdCB3aGljaCB3aWxsIGRldGVjdCBkZWFkbG9jayBpZiBhbnkuCi0g
KiBTZWUgcGFfc2VuZF9kYXRhKCkgYW5kIGVudW0gVHJhbnNBcHBseUFjdGlvbi4KKyAqIHRoZSB0
aW1lb3V0IGlzIGV4Y2VlZGVkLCB0aGUgTEEgd2lsbCBzZXJpYWxpemUgYWxsIHRoZSBwZW5kaW5n
IG1lc3NhZ2VzIHRvCisgKiBhIGZpbGUgYW5kIGluZGljYXRlIFBBLTIgdGhhdCBpdCBuZWVkcyB0
byByZWFkIHRoYXQgZmlsZSBmb3IgdGhlIHJlbWFpbmluZworICogbWVzc2FnZXMuIFRoZW4gTEEg
d2lsbCBzdGFydCB3YWl0aW5nIGZvciBjb21taXQgYXMgaW4gdGhlIHByZXZpb3VzIGNhc2UKKyAq
IHdoaWNoIHdpbGwgZGV0ZWN0IGRlYWRsb2NrIGlmIGFueS4gU2VlIHBhX3NlbmRfZGF0YSgpIGFu
ZAorICogZW51bSBUcmFuc0FwcGx5QWN0aW9uLgogICoKICAqIDQpIExvY2sgdHlwZXMKICAqCkBA
IC01NzUsMTEgKzU3NiwxMyBAQCBwYV9mcmVlX3dvcmtlcihQYXJhbGxlbEFwcGx5V29ya2VySW5m
byAqd2luZm8pCiAJLyoKIAkgKiBTdG9wIHRoZSB3b3JrZXIgaWYgdGhlcmUgYXJlIGVub3VnaCB3
b3JrZXJzIGluIHRoZSBwb29sLgogCSAqCi0JICogWFhYIFRoZSB3b3JrZXIgaXMgYWxzbyBzdG9w
cGVkIGlmIHRoZSBsZWFkZXIgYXBwbHkgd29ya2VyIG5lZWRlZCB0bworCSAqIFhYWCBBZGRpdGlv
bmFsbHksIHdlIGFsc28gc3RvcCB0aGUgd29ya2VyIGlmIHRoZSBsZWFkZXIgYXBwbHkgd29ya2Vy
CiAJICogc2VyaWFsaXplIHBhcnQgb2YgdGhlIHRyYW5zYWN0aW9uIGRhdGEgZHVlIHRvIGEgc2Vu
ZCB0aW1lb3V0LiBUaGlzIGlzCi0JICogYmVjYXVzZSB0aGUgbWVzc2FnZSBjb3VsZCBiZSBwYXJ0
aWFsbHkgd3JpdHRlbiB0byB0aGUgcXVldWUgYnV0IHRoZXJlIGlzCisJICogYmVjYXVzZSB0aGUg
bWVzc2FnZSBjb3VsZCBiZSBwYXJ0aWFsbHkgd3JpdHRlbiB0byB0aGUgcXVldWUgYW5kIHRoZXJl
IGlzCiAJICogbm8gd2F5IHRvIGNsZWFuIHRoZSBxdWV1ZSBvdGhlciB0aGFuIHJlc2VuZGluZyB0
aGUgbWVzc2FnZSB1bnRpbCBpdAotCSAqIHN1Y2NlZWRzLiBEaXJlY3RseSBzdG9wcGluZyB0aGUg
d29ya2VyIGF2b2lkcyBuZWVkaW5nIHRoaXMgY29tcGxleGl0eS4KKwkgKiBzdWNjZWVkcy4gSW5z
dGVhZCBvZiB0cnlpbmcgdG8gc2VuZCB0aGUgZGF0YSB3aGljaCBhbnl3YXkgd291bGQgaGF2ZQor
CSAqIGJlZW4gc2VyaWFsaXplZCBhbmQgdGhlbiBsZXR0aW5nIHRoZSBwYXJhbGxlbCBhcHBseSB3
b3JrZXIgZGVhbCB3aXRoIHRoZQorCSAqIHNwdXJpb3VzIG1lc3NhZ2UsIHdlIHN0b3AgdGhlIHdv
cmtlci4KIAkgKi8KIAlpZiAod2luZm8tPnNlcmlhbGl6ZV9jaGFuZ2VzIHx8CiAJCWxpc3RfbGVu
Z3RoKFBhcmFsbGVsQXBwbHlXb3JrZXJQb29sKSA+CkBAIC02NTIsNyArNjU1LDcgQEAgcGFfZGV0
YWNoX2FsbF9lcnJvcl9tcSh2b2lkKQogfQogCiAvKgotICogQ2hlY2sgaWYgdGhlIHBhcmFsbGVs
IGFwcGx5IHdvcmtlciBpcyBwZW5kaW5nIGR1ZSB0byBzcG9vbGVkIG1lc3NhZ2VzLgorICogQ2hl
Y2sgaWYgdGhlcmUgYXJlIGFueSBwZW5kaW5nIHNwb29sZWQgbWVzc2FnZXMuCiAgKi8KIHN0YXRp
YyBib29sCiBwYV9oYXNfc3Bvb2xlZF9tZXNzYWdlX3BlbmRpbmcoKQpAQCAtNjY1LDggKzY2OCw4
IEBAIHBhX2hhc19zcG9vbGVkX21lc3NhZ2VfcGVuZGluZygpCiB9CiAKIC8qCi0gKiBSZXBsYXkg
dGhlIHNwb29sZWQgbWVzc2FnZXMgaW4gdGhlIHBhcmFsbGVsIGFwcGx5IHdvcmtlciBpZiB0aGUg
bGVhZGVyIGFwcGx5Ci0gKiB3b3JrZXIgaGFzIGZpbmlzaGVkIHNlcmlhbGl6aW5nIGNoYW5nZXMg
dG8gdGhlIGZpbGUuCisgKiBSZXBsYXkgdGhlIHNwb29sZWQgbWVzc2FnZXMgb25jZSB0aGUgbGVh
ZGVyIGFwcGx5IHdvcmtlciBoYXMgZmluaXNoZWQKKyAqIHNlcmlhbGl6aW5nIGNoYW5nZXMgdG8g
dGhlIGZpbGUuCiAgKi8KIHN0YXRpYyB2b2lkCiBwYV9zcG9vbGVkX21lc3NhZ2VzKHZvaWQpCkBA
IC02OTksOCArNzAyLDggQEAgcGFfc3Bvb2xlZF9tZXNzYWdlcyh2b2lkKQogCSAqIFdlIGNhbm5v
dCByZWFkIHRoZSBmaWxlIGltbWVkaWF0ZWx5IGFmdGVyIHRoZSBsZWFkZXIgaGFzIHNlcmlhbGl6
ZWQgYWxsCiAJICogY2hhbmdlcyB0byB0aGUgZmlsZSBiZWNhdXNlIHRoZXJlIG1heSBzdGlsbCBi
ZSBtZXNzYWdlcyBpbiB0aGUgbWVtb3J5CiAJICogcXVldWUuIFdlIHdpbGwgYXBwbHkgYWxsIHNw
b29sZWQgbWVzc2FnZXMgdGhlIG5leHQgdGltZSB3ZSBjYWxsIHRoaXMKLQkgKiBmdW5jdGlvbiwg
d2hpY2ggc2hvdWxkIGVuc3VyZSB0aGF0IHRoZXJlIGFyZSBubyBtZXNzYWdlcyBsZWZ0IGluIHRo
ZQotCSAqIG1lbW9yeSBxdWV1ZS4KKwkgKiBmdW5jdGlvbiBhbmQgdGhhdCB3aWxsIGVuc3VyZSB0
aGVyZSBhcmUgbm8gbWVzc2FnZXMgbGVmdCBpbiB0aGUgbWVtb3J5CisJICogcXVldWUuCiAJICov
CiAJZWxzZSBpZiAoZmlsZXNldF9zdGF0ZSA9PSBGU19TRVJJQUxJWkVfRE9ORSkKIAl7CkBAIC04
MTEsNyArODE0LDcgQEAgTG9naWNhbFBhcmFsbGVsQXBwbHlMb29wKHNobV9tcV9oYW5kbGUgKm1x
aCkKIAkJfQogCQllbHNlIGlmIChzaG1xX3JlcyA9PSBTSE1fTVFfV09VTERfQkxPQ0spCiAJCXsK
LQkJCS8qIENoZWNrIGlmIGNoYW5nZXMgaGF2ZSBiZWVuIHNlcmlhbGl6ZWQgdG8gYSBmaWxlLiAq
LworCQkJLyogUmVwbGF5IHRoZSBjaGFuZ2VzIGZyb20gdGhlIGZpbGUsIGlmIGFueS4gKi8KIAkJ
CWlmIChwYV9oYXNfc3Bvb2xlZF9tZXNzYWdlX3BlbmRpbmcoKSkKIAkJCXsKIAkJCQlwYV9zcG9v
bGVkX21lc3NhZ2VzKCk7CkBAIC0xMjA3LDggKzEyMTAsNyBAQCBwYV9zZW5kX2RhdGEoUGFyYWxs
ZWxBcHBseVdvcmtlckluZm8gKndpbmZvLCBTaXplIG5ieXRlcywgY29uc3Qgdm9pZCAqZGF0YSkK
ICAqIFN3aXRjaCB0byBQQVJUSUFMX1NFUklBTElaRSBtb2RlIGZvciB0aGUgY3VycmVudCB0cmFu
c2FjdGlvbiAtLSB0aGlzIG1lYW5zCiAgKiB0aGF0IHRoZSBjdXJyZW50IGRhdGEgYW5kIGFueSBz
dWJzZXF1ZW50IGRhdGEgZm9yIHRoaXMgdHJhbnNhY3Rpb24gd2lsbCBiZQogICogc2VyaWFsaXpl
ZCB0byBhIGZpbGUuIFRoaXMgaXMgZG9uZSB0byBwcmV2ZW50IHBvc3NpYmxlIGRlYWRsb2NrcyB3
aXRoCi0gKiBhbm90aGVyIHBhcmFsbGVsIGFwcGx5IHdvcmtlciAocmVmZXIgdG8gdGhlIGNvbW1l
bnRzIGF0b3AKLSAqIGFwcGx5cGFyYWxsZWx3b3JrZXIuYyBmb3IgZGV0YWlscykuCisgKiBhbm90
aGVyIHBhcmFsbGVsIGFwcGx5IHdvcmtlciAocmVmZXIgdG8gdGhlIGNvbW1lbnRzIGF0b3AgdGhp
cyBmaWxlKS4KICAqLwogdm9pZAogcGFfc3dpdGNoX3RvX3BhcnRpYWxfc2VyaWFsaXplKFBhcmFs
bGVsQXBwbHlXb3JrZXJJbmZvICp3aW5mbywKQEAgLTEyMTcsNyArMTIxOSw3IEBAIHBhX3N3aXRj
aF90b19wYXJ0aWFsX3NlcmlhbGl6ZShQYXJhbGxlbEFwcGx5V29ya2VySW5mbyAqd2luZm8sCiAJ
LyoKIAkgKiBUaGUgcGFyYWxsZWwgYXBwbHkgd29ya2VyIGNvdWxkIGJlIHN0dWNrIGZvciBzb21l
IHJlYXNvbiAoc2F5IHdhaXRpbmcgb24KIAkgKiBzb21lIGxvY2sgYnkgb3RoZXIgYmFja2VuZCks
IHNvIHN0b3AgdHJ5aW5nIHRvIHNlbmQgZGF0YSBkaXJlY3RseSB0byBpdAotCSAqIGFuZCBzdGFy
dCB0byBzZXJpYWxpemUgZGF0YSB0byBmaWxlIGluc3RlYWQuCisJICogYW5kIHN0YXJ0IHNlcmlh
bGl6aW5nIGRhdGEgdG8gdGhlIGZpbGUgaW5zdGVhZC4KIAkgKi8KIAl3aW5mby0+c2VyaWFsaXpl
X2NoYW5nZXMgPSB0cnVlOwogCkBAIC0xNDg4LDcgKzE0OTAsNyBAQCBwYV9zdHJlYW1fYWJvcnQo
TG9naWNhbFJlcFN0cmVhbUFib3J0RGF0YSAqYWJvcnRfZGF0YSkKIH0KIAogLyoKLSAqIFNldCB0
aGUgZmlsZXNldCBzdGF0ZSBmb3IgdGhlIGdpdmVuIHBhcmFsbGVsIGFwcGx5IHdvcmtlci4gVGhl
IGZpbGVzZXQKKyAqIFNldCB0aGUgZmlsZXNldCBzdGF0ZSBmb3IgYSBwYXJ0aWN1bGFyIHBhcmFs
bGVsIGFwcGx5IHdvcmtlci4gVGhlIGZpbGVzZXQKICAqIHdpbGwgYmUgc2V0IG9uY2UgdGhlIGxl
YWRlciB3b3JrZXIgc2VyaWFsaXplZCBhbGwgY2hhbmdlcyB0byB0aGUgZmlsZQogICogc28gdGhh
dCBpdCBjYW4gYmUgdXNlZCBieSBwYXJhbGxlbCBhcHBseSB3b3JrZXIuCiAgKi8K
--00000000000058201d05f02db9a6--