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: Wed, 17 Jul 2024 17:53:28 +1000
Message-ID: <CAHut+PuEiJSXNv_ixk2+jdxhK4Q3pcfALHoQe7kVnFyCBDdP9Q@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="000000000000a4021e061d6cc360"
Archived-At: <https://www.postgresql.org/message-id/CAHut%2BPuEiJSXNv_ixk2%2BjdxhK4Q3pcfALHoQe7kVnFyCBDdP9Q%40mail.gmail.com>
Precedence: bulk

--000000000000a4021e061d6cc360
Content-Type: multipart/alternative; boundary="000000000000a4021d061d6cc35e"

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

Hi, here are my review comments for v19-0001.

======
doc/src/sgml/protocol.sgml

nitpick - Now there is >1 option. /The following option is supported:/The
following options are supported:/

======
src/backend/access/transam/twophase.c

TwoPhaseTransactionGid:
nitpick - renamed parameter /gid/gid_res/ to emphasize that this is
returned by reference

~~~

1.
IsTwoPhaseTransactionGidForSubid
+ /* Construct the format GID based on the got xid */
+ TwoPhaseTransactionGid(subid, xid, gid_generated, sizeof(gid));

I think the wrong size is being passed here. It should be the buffer size
-- e.g. sizeof(gid_generated).

~

nitpick - renamed a couple of vars for readability
nitpick - expanded some comments.

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

2. AlterSubscription
+ /*
+ * slot_name and two_phase cannot be altered
+ * simultaneously. The latter part refers to the pre-set
+ * slot name and tries to modify the slot option, so
+ * changing both does not make sense.
+ */

I had given a v18-0002 nitpick suggestion to re-word all of this comment.
But, as I wrote before [1], that fix belongs here in patch 0001 where the
comment was first added.

======
[1]
https://www.postgresql.org/message-id/CAHut%2BPsqMRS3dcijo5jsTSbgV1-9So-YBC7PH7xg0%2BZ8hA7fDQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

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

<div dir=3D"ltr">Hi, here are my review comments for v19-0001.<br><br>=3D=
=3D=3D=3D=3D=3D<br>doc/src/sgml/protocol.sgml<br><br>nitpick - Now there is=
 &gt;1 option. /The following option is supported:/The following options ar=
e supported:/<br><br>=3D=3D=3D=3D=3D=3D<br>src/backend/access/transam/twoph=
ase.c<br><br>TwoPhaseTransactionGid:<br>nitpick - renamed parameter /gid/gi=
d_res/ to emphasize that this is returned by reference<br><br>~~~<br><br>1.=
<br>IsTwoPhaseTransactionGidForSubid<br>+	/* Construct the format GID based=
 on the got xid */<br>+	TwoPhaseTransactionGid(subid, xid, gid_generated, s=
izeof(gid));<br><br>I think the wrong size is being passed here. It should =
be the buffer size -- e.g. sizeof(gid_generated).<br><br>~<br><br>nitpick -=
 renamed a couple of vars for readability<br>nitpick - expanded some commen=
ts.<br><br>=3D=3D=3D=3D=3D=3D<br>src/backend/commands/subscriptioncmds.c<br=
><br>2. AlterSubscription<br>+					/*<br>+					 * slot_name and two_phase c=
annot be altered<br>+					 * simultaneously. The latter part refers to the =
pre-set<br>+					 * slot name and tries to modify the slot option, so<br>+	=
				 * changing both does not make sense.<br>+					 */<br><br>I had given a=
 v18-0002 nitpick suggestion to re-word all of this comment. But, as I wrot=
e before [1], that fix belongs here in patch 0001 where the comment was fir=
st added.<br><br>=3D=3D=3D=3D=3D=3D<br>[1] <a href=3D"https://www.postgresq=
l.org/message-id/CAHut%2BPsqMRS3dcijo5jsTSbgV1-9So-YBC7PH7xg0%2BZ8hA7fDQ%40=
mail.gmail.com">https://www.postgresql.org/message-id/CAHut%2BPsqMRS3dcijo5=
jsTSbgV1-9So-YBC7PH7xg0%2BZ8hA7fDQ%40mail.gmail.com</a><br><br>Kind Regards=
,<br>Peter Smith.<br>Fujitsu Australia</div>

--000000000000a4021d061d6cc35e--
--000000000000a4021e061d6cc360
Content-Type: text/plain; charset="US-ASCII"; name="PS_NITPICKS_20240717_v190001.txt"
Content-Disposition: attachment; filename="PS_NITPICKS_20240717_v190001.txt"
Content-Transfer-Encoding: base64
Content-ID: <f_lypjp5130>
X-Attachment-Id: f_lypjp5130

ZGlmZiAtLWdpdCBhL2RvYy9zcmMvc2dtbC9wcm90b2NvbC5zZ21sIGIvZG9jL3NyYy9zZ21sL3By
b3RvY29sLnNnbWwKaW5kZXggY2JhNjY2MS4uMjRjNTdmYiAxMDA2NDQKLS0tIGEvZG9jL3NyYy9z
Z21sL3Byb3RvY29sLnNnbWwKKysrIGIvZG9jL3NyYy9zZ21sL3Byb3RvY29sLnNnbWwKQEAgLTIx
OTIsNyArMjE5Miw3IEBAIHBzcWwgImRibmFtZT1wb3N0Z3JlcyByZXBsaWNhdGlvbj1kYXRhYmFz
ZSIgLWMgIklERU5USUZZX1NZU1RFTTsiCiAgICAgICAgPC92YXJsaXN0ZW50cnk+CiAgICAgICA8
L3ZhcmlhYmxlbGlzdD4KIAotICAgICAgPHBhcmE+VGhlIGZvbGxvd2luZyBvcHRpb24gaXMgc3Vw
cG9ydGVkOjwvcGFyYT4KKyAgICAgIDxwYXJhPlRoZSBmb2xsb3dpbmcgb3B0aW9ucyBhcmUgc3Vw
cG9ydGVkOjwvcGFyYT4KIAogICAgICAgPHZhcmlhYmxlbGlzdD4KICAgICAgICA8dmFybGlzdGVu
dHJ5PgpkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvYWNjZXNzL3RyYW5zYW0vdHdvcGhhc2UuYyBi
L3NyYy9iYWNrZW5kL2FjY2Vzcy90cmFuc2FtL3R3b3BoYXNlLmMKaW5kZXggZjcxMDAzMC4uYjQy
NmYwYiAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvYWNjZXNzL3RyYW5zYW0vdHdvcGhhc2UuYwor
KysgYi9zcmMvYmFja2VuZC9hY2Nlc3MvdHJhbnNhbS90d29waGFzZS5jCkBAIC0yNjg5LDcgKzI2
ODksNyBAQCBMb29rdXBHWGFjdChjb25zdCBjaGFyICpnaWQsIFhMb2dSZWNQdHIgcHJlcGFyZV9l
bmRfbHNuLAogICogUmV0dXJuIHRoZSBHSUQgaW4gdGhlIHN1cHBsaWVkIGJ1ZmZlci4KICAqLwog
dm9pZAotVHdvUGhhc2VUcmFuc2FjdGlvbkdpZChPaWQgc3ViaWQsIFRyYW5zYWN0aW9uSWQgeGlk
LCBjaGFyICpnaWQsIGludCBzemdpZCkKK1R3b1BoYXNlVHJhbnNhY3Rpb25HaWQoT2lkIHN1Ymlk
LCBUcmFuc2FjdGlvbklkIHhpZCwgY2hhciAqZ2lkX3JlcywgaW50IHN6Z2lkKQogewogCUFzc2Vy
dChzdWJpZCAhPSBJbnZhbGlkUmVwT3JpZ2luSWQpOwogCkBAIC0yNjk4LDcgKzI2OTgsNyBAQCBU
d29QaGFzZVRyYW5zYWN0aW9uR2lkKE9pZCBzdWJpZCwgVHJhbnNhY3Rpb25JZCB4aWQsIGNoYXIg
KmdpZCwgaW50IHN6Z2lkKQogCQkJCShlcnJjb2RlKEVSUkNPREVfUFJPVE9DT0xfVklPTEFUSU9O
KSwKIAkJCQkgZXJybXNnX2ludGVybmFsKCJpbnZhbGlkIHR3by1waGFzZSB0cmFuc2FjdGlvbiBJ
RCIpKSk7CiAKLQlzbnByaW50ZihnaWQsIHN6Z2lkLCAicGdfZ2lkXyV1XyV1Iiwgc3ViaWQsIHhp
ZCk7CisJc25wcmludGYoZ2lkX3Jlcywgc3pnaWQsICJwZ19naWRfJXVfJXUiLCBzdWJpZCwgeGlk
KTsKIH0KIAogLyoKQEAgLTI3MTAsMjAgKzI3MTAsMjYgQEAgc3RhdGljIGJvb2wKIElzVHdvUGhh
c2VUcmFuc2FjdGlvbkdpZEZvclN1YmlkKE9pZCBzdWJpZCwgY2hhciAqZ2lkKQogewogCWludAkJ
CXJldDsKLQlPaWQJCQlzdWJpZF93cml0dGVuOwotCVRyYW5zYWN0aW9uSWQgeGlkOworCU9pZAkJ
CXN1YmlkX2Zyb21fZ2lkOworCVRyYW5zYWN0aW9uSWQgeGlkX2Zyb21fZ2lkOwogCWNoYXIJCWdp
ZF9nZW5lcmF0ZWRbR0lEU0laRV07CiAKLQlyZXQgPSBzc2NhbmYoZ2lkLCAicGdfZ2lkXyV1XyV1
IiwgJnN1YmlkX3dyaXR0ZW4sICZ4aWQpOworCS8qIEV4dHJhY3QgdGhlIHN1YmlkIGFuZCB4aWQg
ZnJvbSB0aGUgZ2l2ZW4gR0lELiAqLworCXJldCA9IHNzY2FuZihnaWQsICJwZ19naWRfJXVfJXUi
LCAmc3ViaWRfZnJvbV9naWQsICZ4aWRfZnJvbV9naWQpOwogCi0JLyogUmV0dXJuIGZhbHNlIGlm
IHRoZSBnaXZlbiBHSUQgaGFzIGRpZmZlcmVudCBmb3JtYXQgKi8KLQlpZiAocmV0ICE9IDIgfHwg
c3ViaWQgIT0gc3ViaWRfd3JpdHRlbikKKwkvKgorCSAqIENoZWNrIHRoYXQgdGhlIGdpdmVuIEdJ
RCBoYXMgZXhwZWN0ZWQgZm9ybWF0LCBhbmQgYXQgbGVhc3QgdGhlIHN1YmlkCisJICogbWF0Y2hl
cy4KKwkgKi8KKwlpZiAocmV0ICE9IDIgfHwgc3ViaWQgIT0gc3ViaWRfZnJvbV9naWQpCiAJCXJl
dHVybiBmYWxzZTsKIAotCS8qIENvbnN0cnVjdCB0aGUgZm9ybWF0IEdJRCBiYXNlZCBvbiB0aGUg
Z290IHhpZCAqLwotCVR3b1BoYXNlVHJhbnNhY3Rpb25HaWQoc3ViaWQsIHhpZCwgZ2lkX2dlbmVy
YXRlZCwgc2l6ZW9mKGdpZCkpOwotCi0JLyogLi4uQW5kIGNoZWNrIHdoZXRoZXIgdGhlIGdpdmVu
IEdJRCBpcyBleGFjdCBzYW1lIGFzIHRoZSBmb3JtYXQgR0lEICovCisJLyoKKwkgKiBSZWNvbnN0
cnVjdCBhIHRtcCBHSUQgYmFzZWQgb24gdGhlIHN1YmlkIGFuZCB4aWQgZXh0cmFjdGVkIGZyb20K
KwkgKiB0aGUgZ2l2ZW4gR0lELiBDaGVjayB0aGF0IHRoZSB0bXAgR0lEIGFuZCB0aGUgZ2l2ZW4g
R0lEIG1hdGNoLgorCSAqLworCVR3b1BoYXNlVHJhbnNhY3Rpb25HaWQoc3ViaWRfZnJvbV9naWQs
IHhpZF9mcm9tX2dpZCwKKwkJCQkJCSAgIGdpZF9nZW5lcmF0ZWQsIHNpemVvZihnaWRfZ2VuZXJh
dGVkKSk7CiAJcmV0dXJuIHN0cmNtcChnaWQsIGdpZF9nZW5lcmF0ZWQpID09IDA7CiB9CiAK
--000000000000a4021e061d6cc360--