public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Smith <[email protected]>
To: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Zhijie Hou (Fujitsu) <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Vitaly Davydov <[email protected]>
Cc: Ajin Cherian <[email protected]>
Subject: Re: Slow catchup of 2PC (twophase) transactions on replica in LR
Date: Wed, 17 Jul 2024 17:53:28 +1000
Message-ID: <CAHut+PuEiJSXNv_ixk2+jdxhK4Q3pcfALHoQe7kVnFyCBDdP9Q@mail.gmail.com> (raw)
In-Reply-To: <TYAPR01MB569240C3946E019FB419B6A3F5A32@TYAPR01MB5692.jpnprd01.prod.outlook.com>
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>
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...
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index cba6661..24c57fb 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2192,7 +2192,7 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
</varlistentry>
</variablelist>
- <para>The following option is supported:</para>
+ <para>The following options are supported:</para>
<variablelist>
<varlistentry>
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index f710030..b426f0b 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -2689,7 +2689,7 @@ LookupGXact(const char *gid, XLogRecPtr prepare_end_lsn,
* Return the GID in the supplied buffer.
*/
void
-TwoPhaseTransactionGid(Oid subid, TransactionId xid, char *gid, int szgid)
+TwoPhaseTransactionGid(Oid subid, TransactionId xid, char *gid_res, int szgid)
{
Assert(subid != InvalidRepOriginId);
@@ -2698,7 +2698,7 @@ TwoPhaseTransactionGid(Oid subid, TransactionId xid, char *gid, int szgid)
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg_internal("invalid two-phase transaction ID")));
- snprintf(gid, szgid, "pg_gid_%u_%u", subid, xid);
+ snprintf(gid_res, szgid, "pg_gid_%u_%u", subid, xid);
}
/*
@@ -2710,20 +2710,26 @@ static bool
IsTwoPhaseTransactionGidForSubid(Oid subid, char *gid)
{
int ret;
- Oid subid_written;
- TransactionId xid;
+ Oid subid_from_gid;
+ TransactionId xid_from_gid;
char gid_generated[GIDSIZE];
- ret = sscanf(gid, "pg_gid_%u_%u", &subid_written, &xid);
+ /* Extract the subid and xid from the given GID. */
+ ret = sscanf(gid, "pg_gid_%u_%u", &subid_from_gid, &xid_from_gid);
- /* Return false if the given GID has different format */
- if (ret != 2 || subid != subid_written)
+ /*
+ * Check that the given GID has expected format, and at least the subid
+ * matches.
+ */
+ if (ret != 2 || subid != subid_from_gid)
return false;
- /* Construct the format GID based on the got xid */
- TwoPhaseTransactionGid(subid, xid, gid_generated, sizeof(gid));
-
- /* ...And check whether the given GID is exact same as the format GID */
+ /*
+ * Reconstruct a tmp GID based on the subid and xid extracted from
+ * the given GID. Check that the tmp GID and the given GID match.
+ */
+ TwoPhaseTransactionGid(subid_from_gid, xid_from_gid,
+ gid_generated, sizeof(gid_generated));
return strcmp(gid, gid_generated) == 0;
}
Attachments:
[text/plain] PS_NITPICKS_20240717_v190001.txt (2.4K, ../CAHut+PuEiJSXNv_ixk2+jdxhK4Q3pcfALHoQe7kVnFyCBDdP9Q@mail.gmail.com/3-PS_NITPICKS_20240717_v190001.txt)
download | inline diff:
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index cba6661..24c57fb 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2192,7 +2192,7 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
</varlistentry>
</variablelist>
- <para>The following option is supported:</para>
+ <para>The following options are supported:</para>
<variablelist>
<varlistentry>
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index f710030..b426f0b 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -2689,7 +2689,7 @@ LookupGXact(const char *gid, XLogRecPtr prepare_end_lsn,
* Return the GID in the supplied buffer.
*/
void
-TwoPhaseTransactionGid(Oid subid, TransactionId xid, char *gid, int szgid)
+TwoPhaseTransactionGid(Oid subid, TransactionId xid, char *gid_res, int szgid)
{
Assert(subid != InvalidRepOriginId);
@@ -2698,7 +2698,7 @@ TwoPhaseTransactionGid(Oid subid, TransactionId xid, char *gid, int szgid)
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg_internal("invalid two-phase transaction ID")));
- snprintf(gid, szgid, "pg_gid_%u_%u", subid, xid);
+ snprintf(gid_res, szgid, "pg_gid_%u_%u", subid, xid);
}
/*
@@ -2710,20 +2710,26 @@ static bool
IsTwoPhaseTransactionGidForSubid(Oid subid, char *gid)
{
int ret;
- Oid subid_written;
- TransactionId xid;
+ Oid subid_from_gid;
+ TransactionId xid_from_gid;
char gid_generated[GIDSIZE];
- ret = sscanf(gid, "pg_gid_%u_%u", &subid_written, &xid);
+ /* Extract the subid and xid from the given GID. */
+ ret = sscanf(gid, "pg_gid_%u_%u", &subid_from_gid, &xid_from_gid);
- /* Return false if the given GID has different format */
- if (ret != 2 || subid != subid_written)
+ /*
+ * Check that the given GID has expected format, and at least the subid
+ * matches.
+ */
+ if (ret != 2 || subid != subid_from_gid)
return false;
- /* Construct the format GID based on the got xid */
- TwoPhaseTransactionGid(subid, xid, gid_generated, sizeof(gid));
-
- /* ...And check whether the given GID is exact same as the format GID */
+ /*
+ * Reconstruct a tmp GID based on the subid and xid extracted from
+ * the given GID. Check that the tmp GID and the given GID match.
+ */
+ TwoPhaseTransactionGid(subid_from_gid, xid_from_gid,
+ gid_generated, sizeof(gid_generated));
return strcmp(gid, gid_generated) == 0;
}
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Slow catchup of 2PC (twophase) transactions on replica in LR
In-Reply-To: <CAHut+PuEiJSXNv_ixk2+jdxhK4Q3pcfALHoQe7kVnFyCBDdP9Q@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox