MIME-Version: 1.0
References: <OSBPR01MB255215A39F66932323295854F5DB2@OSBPR01MB2552.jpnprd01.prod.outlook.com>
 <CAA4eK1LOVQ+Qt-FEZPKaPPpggrPrE3AsFXkctp0tDm7CRcHt5Q@mail.gmail.com>
 <TYCPR01MB56938F06B84DD92A88234667F5A22@TYCPR01MB5693.jpnprd01.prod.outlook.com>
 <CAHut+Pv-up=MFqYXSVzCszdxU-n2kdcs3asbrAiKn4_z0VdrrA@mail.gmail.com> <TYAPR01MB569240C3946E019FB419B6A3F5A32@TYAPR01MB5692.jpnprd01.prod.outlook.com>
In-Reply-To: <TYAPR01MB569240C3946E019FB419B6A3F5A32@TYAPR01MB5692.jpnprd01.prod.outlook.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Thu, 18 Jul 2024 09:24:29 +1000
Message-ID: <CAHut+PvmMQHRog+N+hkFCd4g9kHPSJMBfb+NqEbQFJY9dug0bg@mail.gmail.com>
Subject: Re: Slow catchup of 2PC (twophase) transactions on replica in LR
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Amit Kapila <amit.kapila16@gmail.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="00000000000038127f061d79c54b"
Archived-At: <https://www.postgresql.org/message-id/CAHut%2BPvmMQHRog%2BN%2BhkFCd4g9kHPSJMBfb%2BNqEbQFJY9dug0bg%40mail.gmail.com>
Precedence: bulk

--00000000000038127f061d79c54b
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi, here are my review comments for patch v19-0002.

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

CheckAlterSubOption:
nitpick - tweak some comment wording

~

On Wed, Jul 17, 2024 at 3:13=E2=80=AFPM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > 1c.
> > If the error checks can be moved to be done up-front, then all the 'nee=
ds_update'
> > can be combined. Avoiding multiple checks to 'needs_update' will make t=
his function simpler.
>
> This style was needed to preserve error condition for failover option. No=
t changed.
>

nitpick - Hmm. I think you might be trying to preserve the ordering of
errors when that order is of no consequence. AFAICT which error comes
first here is neither documented nor regression tested. e.g.
reordering them gives no problem for testing, but OTOH reordering them
does simplify the code. Anyway, I have modified the code in my
attached nitpicks diffs to demonstrate this suggestion in case you
change your mind.

~~~

AlterSubscription:
nitpick - let's keep all the variables called 'update_xxx' together.
nitpick - comment typo /needs/need/
nitpick - tweak some comment wording

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

nitpick - didn't quite understand the "Since we are..." comment. I
reworded it according to what I thought was the intention.

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

--00000000000038127f061d79c54b
Content-Type: text/plain; charset="US-ASCII"; name="PS_NITPICKS_20240718_2PC_V190002.txt"
Content-Disposition: attachment; 
	filename="PS_NITPICKS_20240718_2PC_V190002.txt"
Content-Transfer-Encoding: base64
Content-ID: <f_lyqgxcs80>
X-Attachment-Id: f_lyqgxcs80

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2NvbW1hbmRzL3N1YnNjcmlwdGlvbmNtZHMuYyBiL3Ny
Yy9iYWNrZW5kL2NvbW1hbmRzL3N1YnNjcmlwdGlvbmNtZHMuYwppbmRleCBmNzcwZTljLi5hODI2
YmU5IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9jb21tYW5kcy9zdWJzY3JpcHRpb25jbWRzLmMK
KysrIGIvc3JjL2JhY2tlbmQvY29tbWFuZHMvc3Vic2NyaXB0aW9uY21kcy5jCkBAIC0xMDc5LDE3
ICsxMDc5LDYgQEAgQ2hlY2tBbHRlclN1Yk9wdGlvbihTdWJzY3JpcHRpb24gKnN1YiwgY29uc3Qg
Y2hhciAqb3B0aW9uLAogCQkgICBzdHJjbXAob3B0aW9uLCAidHdvX3BoYXNlIikgPT0gMCk7CiAK
IAkvKgotCSAqIElmIHRoZSBzbG90IG5lZWRzIHRvIGJlIHVwZGF0ZWQsIHRoZSBiYWNrZW5kIG11
c3QgY29ubmVjdCB0byB0aGUKLQkgKiBwdWJsaXNoZXIgYW5kIHJlcXVlc3QgdGhlIGFsdGVyYXRp
b24uIHNsb3RfbmFtZSBtdXN0IGJlIHJlcXVpcmVkIGF0IHRoYXQKLQkgKiB0aW1lLgotCSAqLwot
CWlmIChzbG90X25lZWRzX3VwZGF0ZSAmJiAhc3ViLT5zbG90bmFtZSkKLQkJZXJlcG9ydChFUlJP
UiwKLQkJCQkoZXJyY29kZShFUlJDT0RFX09CSkVDVF9OT1RfSU5fUFJFUkVRVUlTSVRFX1NUQVRF
KSwKLQkJCQkgZXJybXNnKCJjYW5ub3Qgc2V0ICVzIGZvciBhIHN1YnNjcmlwdGlvbiB0aGF0IGRv
ZXMgbm90IGhhdmUgYSBzbG90IG5hbWUiLAotCQkJCQkJb3B0aW9uKSkpOwotCi0JLyoKIAkgKiBE
byBub3QgYWxsb3cgY2hhbmdpbmcgdGhlIG9wdGlvbiBpZiB0aGUgc3Vic2NyaXB0aW9uIGlzIGVu
YWJsZWQuIFRoaXMKIAkgKiBpcyBiZWNhdXNlIGJvdGggZmFpbG92ZXIgYW5kIHR3b19waGFzZSBv
cHRpb25zIG9mIHRoZSBzbG90IG9uIHRoZQogCSAqIHB1Ymxpc2hlciBjYW5ub3QgYmUgbW9kaWZp
ZWQgaWYgdGhlIHNsb3QgaXMgY3VycmVudGx5IGFjcXVpcmVkIGJ5IHRoZQpAQCAtMTEwMSwxNyAr
MTA5MCwyNyBAQCBDaGVja0FsdGVyU3ViT3B0aW9uKFN1YnNjcmlwdGlvbiAqc3ViLCBjb25zdCBj
aGFyICpvcHRpb24sCiAJCQkJIGVycm1zZygiY2Fubm90IHNldCAlcyBmb3IgZW5hYmxlZCBzdWJz
Y3JpcHRpb24iLAogCQkJCQkJb3B0aW9uKSkpOwogCi0JLyoKLQkgKiBUaGUgY2hhbmdlZCBvcHRp
b24gb2YgdGhlIHNsb3QgY2FuJ3QgYmUgcm9sbGVkIGJhY2ssIHNvIGRpc2FsbG93IGlmIHdlCi0J
ICogYXJlIGluIGEgdHJhbnNhY3Rpb24gYmxvY2suCi0JICovCiAJaWYgKHNsb3RfbmVlZHNfdXBk
YXRlKQogCXsKIAkJU3RyaW5nSW5mb0RhdGEgY21kOwogCisJCS8qCisJCSAqIElmIHRoZSBzbG90
IG5lZWRzIHRvIGJlIHVwZGF0ZWQsIHRoZSBiYWNrZW5kIG11c3QgY29ubmVjdCB0byB0aGUKKwkJ
ICogcHVibGlzaGVyIGFuZCByZXF1ZXN0IHRoZSBhbHRlcmF0aW9uLiBBIHNsb3RfbmFtZSBpcyBy
ZXF1aXJlZCBhdAorCQkgKiB0aGF0IHRpbWUuCisJCSAqLworCQlpZiAoIXN1Yi0+c2xvdG5hbWUp
CisJCQllcmVwb3J0KEVSUk9SLAorCQkJCQkoZXJyY29kZShFUlJDT0RFX09CSkVDVF9OT1RfSU5f
UFJFUkVRVUlTSVRFX1NUQVRFKSwKKwkJCQkJIGVycm1zZygiY2Fubm90IHNldCAlcyBmb3IgYSBz
dWJzY3JpcHRpb24gdGhhdCBkb2VzIG5vdCBoYXZlIGEgc2xvdCBuYW1lIiwKKwkJCQkJCQlvcHRp
b24pKSk7CisKKwkJLyoKKwkJICogVGhlIGNoYW5nZWQgb3B0aW9uIG9mIHRoZSBzbG90IGNhbid0
IGJlIHJvbGxlZCBiYWNrLCBzbyBkaXNhbGxvdyBpZiB3ZQorCQkgKiBhcmUgaW4gYSB0cmFuc2Fj
dGlvbiBibG9jay4KKwkJICovCiAJCWluaXRTdHJpbmdJbmZvKCZjbWQpOwogCQlhcHBlbmRTdHJp
bmdJbmZvKCZjbWQsICJBTFRFUiBTVUJTQ1JJUFRJT04gLi4uIFNFVCAoJXMpIiwgb3B0aW9uKTsK
LQogCQlQcmV2ZW50SW5UcmFuc2FjdGlvbkJsb2NrKGlzVG9wTGV2ZWwsIGNtZC5kYXRhKTsKIAkJ
cGZyZWUoY21kLmRhdGEpOwogCX0KQEAgLTExMzIsMTIgKzExMzEsMTIgQEAgQWx0ZXJTdWJzY3Jp
cHRpb24oUGFyc2VTdGF0ZSAqcHN0YXRlLCBBbHRlclN1YnNjcmlwdGlvblN0bXQgKnN0bXQsCiAJ
SGVhcFR1cGxlCXR1cDsKIAlPaWQJCQlzdWJpZDsKIAlib29sCQl1cGRhdGVfdHVwbGUgPSBmYWxz
ZTsKKwlib29sCQl1cGRhdGVfZmFpbG92ZXIgPSBmYWxzZTsKKwlib29sCQl1cGRhdGVfdHdvX3Bo
YXNlID0gZmFsc2U7CiAJU3Vic2NyaXB0aW9uICpzdWI7CiAJRm9ybV9wZ19zdWJzY3JpcHRpb24g
Zm9ybTsKIAliaXRzMzIJCXN1cHBvcnRlZF9vcHRzOwogCVN1Yk9wdHMJCW9wdHMgPSB7MH07Ci0J
Ym9vbAkJdXBkYXRlX2ZhaWxvdmVyID0gZmFsc2U7Ci0JYm9vbAkJdXBkYXRlX3R3b19waGFzZSA9
IGZhbHNlOwogCiAJcmVsID0gdGFibGVfb3BlbihTdWJzY3JpcHRpb25SZWxhdGlvbklkLCBSb3dF
eGNsdXNpdmVMb2NrKTsKIApAQCAtMTI3MSw3ICsxMjcwLDcgQEAgQWx0ZXJTdWJzY3JpcHRpb24o
UGFyc2VTdGF0ZSAqcHN0YXRlLCBBbHRlclN1YnNjcmlwdGlvblN0bXQgKnN0bXQsCiAJCQkJaWYg
KElzU2V0KG9wdHMuc3BlY2lmaWVkX29wdHMsIFNVQk9QVF9GQUlMT1ZFUikpCiAJCQkJewogCQkJ
CQkvKgotCQkJCQkgKiBGaXJzdCwgbWFyayB0aGUgbmVlZHMgdG8gYWx0ZXIgdGhlIHJlcGxpY2F0
aW9uIHNsb3QuCisJCQkJCSAqIEZpcnN0LCBtYXJrIHRoZSBuZWVkIHRvIGFsdGVyIHRoZSByZXBs
aWNhdGlvbiBzbG90LgogCQkJCQkgKiBGYWlsb3ZlciBvcHRpb24gaXMgY29udHJvbGxlZCBieSBi
b3RoIHRoZSBwdWJsaXNoZXIgKGFzCiAJCQkJCSAqIGEgc2xvdCBvcHRpb24pIGFuZCB0aGUgc3Vi
c2NyaWJlciAoYXMgYSBzdWJzY3JpcHRpb24KIAkJCQkJICogb3B0aW9uKS4KQEAgLTEyOTUsMTUg
KzEyOTQsMTQgQEAgQWx0ZXJTdWJzY3JpcHRpb24oUGFyc2VTdGF0ZSAqcHN0YXRlLCBBbHRlclN1
YnNjcmlwdGlvblN0bXQgKnN0bXQsCiAJCQkJCSAqIHN1YnNjcmlwdGlvbiBvcHRpb24pLiBUaGUg
c2xvdCBvcHRpb24gbXVzdCBiZSBhbHRlcmVkCiAJCQkJCSAqIG9ubHkgd2hlbiBjaGFuZ2luZyAi
dHJ1ZSIgdG8gImZhbHNlIi4KIAkJCQkJICoKLQkJCQkJICogVGhlcmUgaXMgbm8gbmVlZCB0byBk
byBzb21ldGhpbmcgcmVtYXJrYWJsZSByZWdhcmRpbmcKKwkJCQkJICogVGhlcmUgaXMgbm8gbmVl
ZCB0byBkbyBhbnl0aGluZyByZW1hcmthYmxlIGZvcgogCQkJCQkgKiB0aGUgImZhbHNlIiB0byAi
dHJ1ZSIgY2FzZTsgdGhlIGJhY2tlbmQgcHJvY2VzcyBhbHRlcnMKIAkJCQkJICogc3VidHdvcGhh
c2UgdG8gTE9HSUNBTFJFUF9UV09QSEFTRV9TVEFURV9QRU5ESU5HIG9uY2UuCiAJCQkJCSAqIEFm
dGVyIHRoZSBzdWJzY3JpcHRpb24gaXMgZW5hYmxlZCwgYSBuZXcgbG9naWNhbAogCQkJCQkgKiBy
ZXBsaWNhdGlvbiB3b3JrZXIgcmVxdWVzdHMgdG8gY2hhbmdlIHRoZSB0d29fcGhhc2UKIAkJCQkJ
ICogb3B0aW9uIG9mIGl0cyBzbG90IGZyb20gcGVuZGluZyB0byB0cnVlIHdoZW4gdGhlCi0JCQkJ
CSAqIGluaXRpYWwgZGF0YSBzeW5jaHJvbml6YXRpb24gaXMgZG9uZS4gVGhlIGNvZGUgcGF0aCBp
cwotCQkJCQkgKiB0aGUgc2FtZSBhcyB0aGUgY2FzZSBpbiB3aGljaCB0d29fcGhhc2UgaXMgaW5p
dGlhbGx5Ci0JCQkJCSAqIHNldCB0byB0cnVlLgorCQkJCQkgKiBpbml0aWFsIGRhdGEgc3luY2hy
b25pemF0aW9uIGlzIGRvbmUuIFRoYXQgY29kZSBwYXRoIGlzCisJCQkJCSAqIHRoZSBzYW1lIGFz
IHdoZW4gdHdvX3BoYXNlIGlzIGluaXRpYWxseSBzZXQgdG8gdHJ1ZS4KIAkJCQkJICovCiAJCQkJ
CXVwZGF0ZV90d29fcGhhc2UgPSAhb3B0cy50d29waGFzZTsKIApkaWZmIC0tZ2l0IGEvc3JjL3Rl
c3Qvc3Vic2NyaXB0aW9uL3QvMDIxX3R3b3BoYXNlLnBsIGIvc3JjL3Rlc3Qvc3Vic2NyaXB0aW9u
L3QvMDIxX3R3b3BoYXNlLnBsCmluZGV4IGM4ZmIzYjcuLjkxZTJlMTcgMTAwNjQ0Ci0tLSBhL3Ny
Yy90ZXN0L3N1YnNjcmlwdGlvbi90LzAyMV90d29waGFzZS5wbAorKysgYi9zcmMvdGVzdC9zdWJz
Y3JpcHRpb24vdC8wMjFfdHdvcGhhc2UucGwKQEAgLTQyOCw5ICs0MjgsOCBAQCBpcygkcmVzdWx0
LCBxcSgwKSwgJ3Nob3VsZCBiZSBubyBwcmVwYXJlZCB0cmFuc2FjdGlvbnMgb24gc3Vic2NyaWJl
cicpOwogCiAjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjCiAjIFRvZ2dsZSB0aGUgdHdv
X3BoYXNlIHRvICJ0cnVlIiBiZWZvcmUgdGhlIENPTU1JVCBQUkVQQVJFRC4KLSMKLSMgU2luY2Ug
d2UgYXJlIHRoZSBzcGVjaWFsIHBhdGggZm9yIHRoZSBjYXNlIHdoZXJlIGJvdGggdHdvX3BoYXNl
Ci0jIGFuZCBmYWlsb3ZlciBhcmUgYWx0ZXJlZCwgaXQgaXMgYWxzbyBzZXQgdG8gInRydWUiLgor
IyBBbHNvLCBzZXQgZmFpbG92ZXIgdG8gInRydWUiIHRvIHRlc3QgdGhlIGNvZGUgcGF0aCB3aGVy
ZQorIyBib3RoIHR3b19waGFzZSBhbmQgZmFpbG92ZXIgYXJlIGFsdGVyZWQgYXQgdGhlIHNhbWUg
dGltZS4KICMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMKICRub2RlX3N1YnNjcmliZXIt
PnNhZmVfcHNxbCgncG9zdGdyZXMnLAogICAgICJBTFRFUiBTVUJTQ1JJUFRJT04gdGFwX3N1Yl9j
b3B5IERJU0FCTEU7Iik7Cg==
--00000000000038127f061d79c54b--