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
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;
}
