MIME-Version: 1.0
References: <OSBPR01MB255215A39F66932323295854F5DB2@OSBPR01MB2552.jpnprd01.prod.outlook.com>
 <CAA4eK1LOVQ+Qt-FEZPKaPPpggrPrE3AsFXkctp0tDm7CRcHt5Q@mail.gmail.com> <TYCPR01MB56938F06B84DD92A88234667F5A22@TYCPR01MB5693.jpnprd01.prod.outlook.com>
In-Reply-To: <TYCPR01MB56938F06B84DD92A88234667F5A22@TYCPR01MB5693.jpnprd01.prod.outlook.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Wed, 17 Jul 2024 13:31:14 +1000
Message-ID: <CAHut+PsqMRS3dcijo5jsTSbgV1-9So-YBC7PH7xg0+Z8hA7fDQ@mail.gmail.com>
Subject: Re: Slow catchup of 2PC (twophase) transactions on replica in LR
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, 
	"Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, 
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>, 
	Vitaly Davydov <v.davydov@postgrespro.ru>, Ajin Cherian <itsajin@gmail.com>
Content-Type: multipart/mixed; boundary="000000000000cfa844061d69193e"
Archived-At: <https://www.postgresql.org/message-id/CAHut%2BPsqMRS3dcijo5jsTSbgV1-9So-YBC7PH7xg0%2BZ8hA7fDQ%40mail.gmail.com>
Precedence: bulk

--000000000000cfa844061d69193e
Content-Type: multipart/alternative; boundary="000000000000cfa842061d69193c"

--000000000000cfa842061d69193c
Content-Type: text/plain; charset="UTF-8"

Here are some review comments for patch v18-0002.

======
src/backend/commands/subscriptioncmds.c

1. CheckAlterSubOption

1a.
It's not obvious why we are only checking the 'slot name' when
needs_update==true, but OTOH is always checking the  'enabled' state.

~

1b.
Param 'needs_update' is a vague name. It needs more explanatory comments or
a better name. e.g. First impression was "Why are we calling 'Alter'
function if needs_update is false?". I know it encapsulates some common
code, but if special cases cause the logic to be more confusing then that
cost may outweigh the benefit of this function.

~

1c.
If the error checks can be moved to be done up-front, then all the
'needs_update' can be combined. Avoiding multiple checks to 'needs_update'
will make this function simpler.

~~~

AlterSubscription:
nitpick - typo /needs/need/
nitpick - typo /wo_phase/two_phase/
nitpick - The comment wording "the later part...", was confusing. I've
reworded the whole comment. But this belongs in patch 0001.

======
src/test/subscription/t/021_twophase.pl

nitpick - Use the same "###############################" comment style as
in patch 0001 to indicate each main TEST scenario.

======
99.
Please refer to the diffs attachment patch, which implements any nitpicks
mentioned above.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

--000000000000cfa842061d69193c
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Here are some review comments for patch v18-0002.<br><br>=
=3D=3D=3D=3D=3D=3D<br>src/backend/commands/subscriptioncmds.c <br><br>1. Ch=
eckAlterSubOption<br><br>1a.<br>It&#39;s not obvious why we are only checki=
ng the &#39;slot name&#39; when needs_update=3D=3Dtrue, but OTOH is always =
checking=C2=A0the=C2=A0 &#39;enabled&#39; state.<br><br>~<br><br>1b.<br>Par=
am &#39;needs_update&#39; is a vague name. It needs more explanatory commen=
ts or a better name. e.g. First impression was &quot;Why are we calling =
9;Alter&#39; function if needs_update is false?&quot;. I know it encapsulat=
es some common code, but if special cases cause the logic to be more confus=
ing then that cost may outweigh the benefit of this function.<br><br>~<br><=
br>1c.<br>If the error checks can be moved to be done up-front, then all th=
e &#39;needs_update&#39; can be combined. Avoiding multiple checks to &#39;=
needs_update&#39; will make this function simpler.<br>=C2=A0<br>~~~<br><br>=
AlterSubscription:<br>nitpick - typo /needs/need/<br>nitpick - typo /wo_pha=
se/two_phase/<br>nitpick - The comment wording &quot;the later part...&quot=
;, was confusing. I&#39;ve reworded the whole comment. But this belongs in =
patch 0001.<br><br>=3D=3D=3D=3D=3D=3D<br>src/test/subscription/t/<a href=3D=
"http://021_twophase.pl">021_twophase.pl</a> <br><br>nitpick - Use the same=
 &quot;###############################&quot; comment style as in patch 0001=
 to indicate each main TEST scenario.<br><br>=3D=3D=3D=3D=3D=3D<br>99.<br>P=
lease refer to the diffs attachment patch, which implements any nitpicks me=
ntioned above.<br><br>=3D=3D=3D=3D=3D=3D<br>Kind Regards,<br>Peter Smith.<b=
r>Fujitsu Australia<br></div>

--000000000000cfa842061d69193c--
--000000000000cfa844061d69193e
Content-Type: text/plain; charset="US-ASCII"; name="PS_NITPICKS_20240717_2PC_V180002.txt"
Content-Disposition: attachment; 
	filename="PS_NITPICKS_20240717_2PC_V180002.txt"
Content-Transfer-Encoding: base64
Content-ID: <f_lypac5ba0>
X-Attachment-Id: f_lypac5ba0

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2NvbW1hbmRzL3N1YnNjcmlwdGlvbmNtZHMuYyBiL3Ny
Yy9iYWNrZW5kL2NvbW1hbmRzL3N1YnNjcmlwdGlvbmNtZHMuYwppbmRleCBkODBkNjBjLi41YzZm
MGE1IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9jb21tYW5kcy9zdWJzY3JpcHRpb25jbWRzLmMK
KysrIGIvc3JjL2JhY2tlbmQvY29tbWFuZHMvc3Vic2NyaXB0aW9uY21kcy5jCkBAIC0xMjYyLDcg
KzEyNjIsNyBAQCBBbHRlclN1YnNjcmlwdGlvbihQYXJzZVN0YXRlICpwc3RhdGUsIEFsdGVyU3Vi
c2NyaXB0aW9uU3RtdCAqc3RtdCwKIAkJCQlpZiAoSXNTZXQob3B0cy5zcGVjaWZpZWRfb3B0cywg
U1VCT1BUX0ZBSUxPVkVSKSkKIAkJCQl7CiAJCQkJCS8qCi0JCQkJCSAqIEZpcnN0IG1hcmsgdGhl
IG5lZWRzIHRvIGFsdGVyIHRoZSByZXBsaWNhdGlvbiBzbG90LgorCQkJCQkgKiBGaXJzdCwgbWFy
ayB0aGUgbmVlZCB0byBhbHRlciB0aGUgcmVwbGljYXRpb24gc2xvdC4KIAkJCQkJICogRmFpbG92
ZXIgb3B0aW9uIGlzIGNvbnRyb2xsZWQgYnkgYm90aCB0aGUgcHVibGlzaGVyIChhcwogCQkJCQkg
KiBhIHNsb3Qgb3B0aW9uKSBhbmQgdGhlIHN1YnNjcmliZXIgKGFzIGEgc3Vic2NyaXB0aW9uCiAJ
CQkJCSAqIG9wdGlvbikuCkBAIC0xMjgwLDcgKzEyODAsNyBAQCBBbHRlclN1YnNjcmlwdGlvbihQ
YXJzZVN0YXRlICpwc3RhdGUsIEFsdGVyU3Vic2NyaXB0aW9uU3RtdCAqc3RtdCwKIAkJCQlpZiAo
SXNTZXQob3B0cy5zcGVjaWZpZWRfb3B0cywgU1VCT1BUX1RXT1BIQVNFX0NPTU1JVCkpCiAJCQkJ
ewogCQkJCQkvKgotCQkJCQkgKiBGaXJzdCBjaGVjayB0aGUgbmVlZCB0byBhbHRlciB0aGUgcmVw
bGljYXRpb24gc2xvdC4KKwkJCQkJICogRmlyc3QsIGNoZWNrIHRoZSBuZWVkIHRvIGFsdGVyIHRo
ZSByZXBsaWNhdGlvbiBzbG90LgogCQkJCQkgKiBUd29fcGhhc2Ugb3B0aW9uIGlzIGNvbnRyb2xs
ZWQgYnkgYm90aCB0aGUgcHVibGlzaGVyCiAJCQkJCSAqIChhcyBhIHNsb3Qgb3B0aW9uKSBhbmQg
dGhlIHN1YnNjcmliZXIgKGFzIGEKIAkJCQkJICogc3Vic2NyaXB0aW9uIG9wdGlvbikuIFRoZSBz
bG90IG9wdGlvbiBtdXN0IGJlIGFsdGVyZWQKQEAgLTEzMDIsMTEgKzEzMDIsOSBAQCBBbHRlclN1
YnNjcmlwdGlvbihQYXJzZVN0YXRlICpwc3RhdGUsIEFsdGVyU3Vic2NyaXB0aW9uU3RtdCAqc3Rt
dCwKIAkJCQkJCQkJCQlpc1RvcExldmVsKTsKIAogCQkJCQkvKgotCQkJCQkgKiBJZiB0aGUgd29f
cGhhc2Ugc2xvdCBvcHRpb24gbXVzdCBiZSBhbHRlcmVkLCB0aGlzCi0JCQkJCSAqIGNhbm5vdCBi
ZSBhbHRlcmVkIHdpdGggc2xvdF9uYW1lIHNpbXVsdGFuZW91c2x5LiBUaGUKLQkJCQkJICogbGF0
dGVyIHBhcnQgcmVmZXJzIHRvIHRoZSBwcmUtc2V0IHNsb3QgbmFtZSBhbmQgdHJpZXMKLQkJCQkJ
ICogdG8gbW9kaWZ5IHRoZSBzbG90IG9wdGlvbiwgc28gY2hhbmdpbmcgYm90aCBkb2VzIG5vdAot
CQkJCQkgKiBtYWtlIHNlbnNlLgorCQkJCQkgKiBNb2RpZnlpbmcgdGhlIHR3b19waGFzZSBzbG90
IG9wdGlvbiByZXF1aXJlcyBhIHNsb3QKKwkJCQkJICogbG9va3VwIGJ5IHNsb3QgbmFtZSwgc28g
Y2hhbmdpbmcgdGhlIHNsb3QgbmFtZQorCQkJCQkgKiBhdCB0aGUgc2FtZSB0aW1lIGlzIG5vdCBh
bGxvd2VkLgogCQkJCQkgKi8KIAkJCQkJaWYgKHVwZGF0ZV90d29fcGhhc2UgJiYKIAkJCQkJCUlz
U2V0KG9wdHMuc3BlY2lmaWVkX29wdHMsIFNVQk9QVF9TTE9UX05BTUUpKQpkaWZmIC0tZ2l0IGEv
c3JjL3Rlc3Qvc3Vic2NyaXB0aW9uL3QvMDIxX3R3b3BoYXNlLnBsIGIvc3JjL3Rlc3Qvc3Vic2Ny
aXB0aW9uL3QvMDIxX3R3b3BoYXNlLnBsCmluZGV4IGY1NmRmZjQuLjI2YjllMmMgMTAwNjQ0Ci0t
LSBhL3NyYy90ZXN0L3N1YnNjcmlwdGlvbi90LzAyMV90d29waGFzZS5wbAorKysgYi9zcmMvdGVz
dC9zdWJzY3JpcHRpb24vdC8wMjFfdHdvcGhhc2UucGwKQEAgLTQyMyw5ICs0MjMsMTIgQEAgJHJl
c3VsdCA9ICRub2RlX3N1YnNjcmliZXItPnNhZmVfcHNxbCgncG9zdGdyZXMnLAogCSJTRUxFQ1Qg
Y291bnQoKikgRlJPTSBwZ19wcmVwYXJlZF94YWN0czsiKTsKIGlzKCRyZXN1bHQsIHFxKDApLCAn
c2hvdWxkIGJlIG5vIHByZXBhcmVkIHRyYW5zYWN0aW9ucyBvbiBzdWJzY3JpYmVyJyk7CiAKLSMg
VG9nZ2xlIHRoZSB0d29fcGhhc2UgdG8gInRydWUiICpiZWZvcmUqIHRoZSBDT01NSVQgUFJFUEFS
RUQuIFNpbmNlIHdlIGFyZSB0aGUKLSMgc3BlY2lhbCBwYXRoIGZvciB0aGUgY2FzZSB3aGVyZSBi
b3RoIHR3b19waGFzZSBhbmQgZmFpbG92ZXIgYXJlIGFsdGVyZWQsIGl0Ci0jIGlzIGFsc28gc2V0
IHRvICJ0cnVlIi4KKyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMKKyMgVG9nZ2xlIHRo
ZSB0d29fcGhhc2UgdG8gInRydWUiIGJlZm9yZSB0aGUgQ09NTUlUIFBSRVBBUkVELgorIworIyBT
aW5jZSB3ZSBhcmUgdGhlIHNwZWNpYWwgcGF0aCBmb3IgdGhlIGNhc2Ugd2hlcmUgYm90aCB0d29f
cGhhc2UKKyMgYW5kIGZhaWxvdmVyIGFyZSBhbHRlcmVkLCBpdCBpcyBhbHNvIHNldCB0byAidHJ1
ZSIuCisjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjCiAkbm9kZV9zdWJzY3JpYmVyLT5z
YWZlX3BzcWwoJ3Bvc3RncmVzJywKICAgICAiQUxURVIgU1VCU0NSSVBUSU9OIHRhcF9zdWJfY29w
eSBESVNBQkxFOyIpOwogJG5vZGVfc3Vic2NyaWJlci0+cG9sbF9xdWVyeV91bnRpbCgncG9zdGdy
ZXMnLAo=
--000000000000cfa844061d69193e--