Date: Tue, 7 Jul 2020 14:35:48 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "movead.li@highgo.ca" <movead.li@highgo.ca>
Cc: petr <petr@2ndquadrant.com>, simon <simon@2ndquadrant.com>,
	"Madan Kumar" <madankumar1993@gmail.com>,
	pgsql-hackers <pgsql-hackers@lists.postgresql.org>,
	craig <craig@2ndquadrant.com>,
	"Alvaro Herrera" <alvherre@2ndquadrant.com>,
	andres <andres@anarazel.de>,
	soumyadeep2007 <soumyadeep2007@gmail.com>
Subject: Re: A patch for get origin from commit_ts.
Message-ID: <20200707053548.GA3605@paquier.xyz>
References: 
 <CAMtsaPEm47-mD5br8O8fVsZ0PnowtNij4O3UNsuG6Zb7dSCAaQ@mail.gmail.com>
 <CANP8+jJu-bta+4KBRDf2XKsHybDBY1Ew_zvcuoTdhES+ZWLGcQ@mail.gmail.com>
 <20200702015852.GE10408@paquier.xyz>
 <4ee522fa-b549-4a8d-9d9e-cb27be774627@2ndquadrant.com>
 <20200703051059.GB1729@paquier.xyz>
 <202007041801258116499@highgo.ca>
 <20200706003635.GC2143@paquier.xyz>
 <202007061112287118394@highgo.ca>
 <20200706080136.GH2143@paquier.xyz>
 <2020070710022545488010@highgo.ca>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="TakKZr9L6Hm6aLOc"
Content-Disposition: inline
In-Reply-To: <2020070710022545488010@highgo.ca>
Precedence: bulk


--TakKZr9L6Hm6aLOc
Content-Type: multipart/mixed; boundary="d6Gm4EdcadzBjdND"
Content-Disposition: inline


--d6Gm4EdcadzBjdND
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Jul 07, 2020 at 10:02:29AM +0800, movead.li@highgo.ca wrote:
> Thanks, very helpful information and I have followed that.

Cool, thanks.  I have gone through your patch in details, and updated
it as the attached.  Here are some comments.

'8000' as OID for the new function was not really random, so to be
fair with the other patches, I picked up the first random value
unused_oids has given me instead.

There were some indentation issues, and pgindent got that fixed.

I think that it is better to use "replication origin" in the docs
instead of just origin.  I have kept "origin" in the functions for
now as that sounded cleaner to me, but we may consider using something
like "reporigin" as well as attribute name.

The tests could just use tstzrange() to make sure that the timestamps
have valid values, so I have switched to that, and did not resist to
do the same in the existing tests.

+-- Test when it can not find the transaction
+SELECT * FROM pg_xact_commit_timestamp_origin((:'txid_set_origin'::text::int +
10)::text::xid) x;
This test could become unstable, particularly if it gets used in a
parallel environment, so I have removed it.  Perhaps I am just
over-pessimistic here though..

As a side note, I think that we could just remove the alternate output
of commit_ts/, as it does not really get used because of the
NO_INSTALLCHECK present in the module's Makefile.  That would be the
job of a different patch, so I have updated it accordingly.  Glad to
see that you did not forget to adapt it in your own patch.

(The change in catversion.h is a self-reminder...)
--
Michael

--d6Gm4EdcadzBjdND
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="get_origin_from_commit_ts_v6.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catvers=
ion.h
index 54518cd40e..ee58586569 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */
=20
 /*							yyyymmddN */
-#define CATALOG_VERSION_NO	202007061
+#define CATALOG_VERSION_NO	202007121
=20
 #endif
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.=
dat
index 38295aca48..585823576a 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5946,12 +5946,20 @@
   prorettype =3D> 'timestamptz', proargtypes =3D> 'xid',
   prosrc =3D> 'pg_xact_commit_timestamp' },
=20
+{ oid =3D> '8456',
+  descr =3D> 'get commit timestamp and replication origin of a transaction=
',
+  proname =3D> 'pg_xact_commit_timestamp_origin', provolatile =3D> 'v',
+  prorettype =3D> 'record', proargtypes =3D> 'xid',
+  proallargtypes =3D> '{xid,timestamptz,oid}', proargmodes =3D> '{i,o,o}',
+  proargnames =3D> '{xid,timestamp,origin}',
+  prosrc =3D> 'pg_xact_commit_timestamp_origin' },
+
 { oid =3D> '3583',
-  descr =3D> 'get transaction Id and commit timestamp of latest transactio=
n commit',
+  descr =3D> 'get transaction Id, commit timestamp and replication origin =
of latest transaction commit',
   proname =3D> 'pg_last_committed_xact', provolatile =3D> 'v',
   prorettype =3D> 'record', proargtypes =3D> '',
-  proallargtypes =3D> '{xid,timestamptz}', proargmodes =3D> '{o,o}',
-  proargnames =3D> '{xid,timestamp}', prosrc =3D> 'pg_last_committed_xact'=
 },
+  proallargtypes =3D> '{xid,timestamptz,oid}', proargmodes =3D> '{o,o,o}',
+  proargnames =3D> '{xid,timestamp,origin}', prosrc =3D> 'pg_last_committe=
d_xact' },
=20
 { oid =3D> '3537', descr =3D> 'get identification of SQL object',
   proname =3D> 'pg_describe_object', provolatile =3D> 's', prorettype =3D>=
 'text',
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/tr=
ansam/commit_ts.c
index 9cdb136435..e48f88c884 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -417,28 +417,38 @@ pg_xact_commit_timestamp(PG_FUNCTION_ARGS)
 }
=20
=20
+/*
+ * pg_last_committed_xact
+ *
+ * SQL-callable wrapper to obtain some information about the latest
+ * committed transaction: transaction ID, timestamp and replication
+ * origin.
+ */
 Datum
 pg_last_committed_xact(PG_FUNCTION_ARGS)
 {
 	TransactionId xid;
+	RepOriginId nodeid;
 	TimestampTz ts;
-	Datum		values[2];
-	bool		nulls[2];
+	Datum		values[3];
+	bool		nulls[3];
 	TupleDesc	tupdesc;
 	HeapTuple	htup;
=20
 	/* and construct a tuple with our data */
-	xid =3D GetLatestCommitTsData(&ts, NULL);
+	xid =3D GetLatestCommitTsData(&ts, &nodeid);
=20
 	/*
 	 * Construct a tuple descriptor for the result row.  This must match this
 	 * function's pg_proc entry!
 	 */
-	tupdesc =3D CreateTemplateTupleDesc(2);
+	tupdesc =3D CreateTemplateTupleDesc(3);
 	TupleDescInitEntry(tupdesc, (AttrNumber) 1, "xid",
 					   XIDOID, -1, 0);
 	TupleDescInitEntry(tupdesc, (AttrNumber) 2, "timestamp",
 					   TIMESTAMPTZOID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 3, "origin",
+					   OIDOID, -1, 0);
 	tupdesc =3D BlessTupleDesc(tupdesc);
=20
 	if (!TransactionIdIsNormal(xid))
@@ -452,6 +462,9 @@ pg_last_committed_xact(PG_FUNCTION_ARGS)
=20
 		values[1] =3D TimestampTzGetDatum(ts);
 		nulls[1] =3D false;
+
+		values[2] =3D ObjectIdGetDatum(nodeid);
+		nulls[2] =3D false;
 	}
=20
 	htup =3D heap_form_tuple(tupdesc, values, nulls);
@@ -459,6 +472,54 @@ pg_last_committed_xact(PG_FUNCTION_ARGS)
 	PG_RETURN_DATUM(HeapTupleGetDatum(htup));
 }
=20
+/*
+ * pg_xact_commit_timestamp_origin
+ *
+ * SQL-callable wrapper to obtain commit timestamp and origin of a given
+ * transaction.
+ */
+Datum
+pg_xact_commit_timestamp_origin(PG_FUNCTION_ARGS)
+{
+	TransactionId xid =3D PG_GETARG_UINT32(0);
+	RepOriginId nodeid;
+	TimestampTz ts;
+	Datum		values[2];
+	bool		nulls[2];
+	TupleDesc	tupdesc;
+	HeapTuple	htup;
+	bool		found;
+
+	found =3D TransactionIdGetCommitTsData(xid, &ts, &nodeid);
+
+	/*
+	 * Construct a tuple descriptor for the result row.  This must match this
+	 * function's pg_proc entry!
+	 */
+	tupdesc =3D CreateTemplateTupleDesc(2);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 1, "timestamp",
+					   TIMESTAMPTZOID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 2, "origin",
+					   OIDOID, -1, 0);
+	tupdesc =3D BlessTupleDesc(tupdesc);
+
+	if (!found)
+	{
+		nulls[0] =3D true;
+		nulls[1] =3D true;
+	}
+	else
+	{
+		values[0] =3D TimestampTzGetDatum(ts);
+		nulls[0] =3D false;
+		values[1] =3D ObjectIdGetDatum(nodeid);
+		nulls[1] =3D false;
+	}
+
+	htup =3D heap_form_tuple(tupdesc, values, nulls);
+
+	PG_RETURN_DATUM(HeapTupleGetDatum(htup));
+}
=20
 /*
  * Number of shared CommitTS buffers.
diff --git a/src/test/modules/commit_ts/expected/commit_timestamp.out b/src=
/test/modules/commit_ts/expected/commit_timestamp.out
index 5b7783b58f..973750c13a 100644
--- a/src/test/modules/commit_ts/expected/commit_timestamp.out
+++ b/src/test/modules/commit_ts/expected/commit_timestamp.out
@@ -39,9 +39,89 @@ SELECT pg_xact_commit_timestamp('2'::xid);
 =20
 (1 row)
=20
-SELECT x.xid::text::bigint > 0, x.timestamp > '-infinity'::timestamptz, x.=
timestamp <=3D now() FROM pg_last_committed_xact() x;
- ?column? | ?column? | ?column?=20
-----------+----------+----------
- t        | t        | t
+SELECT x.xid::text::bigint > 0 as xid_valid,
+    x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_ran=
ge,
+    origin
+  FROM pg_last_committed_xact() x;
+ xid_valid | ts_in_range | origin=20
+-----------+-------------+--------
+ t         | t           |      0
+(1 row)
+
+-- Test non-normal transaction ids.
+SELECT * FROM pg_xact_commit_timestamp_origin(NULL); -- ok, NULL
+ timestamp | origin=20
+-----------+--------
+           |      =20
+(1 row)
+
+SELECT * FROM pg_xact_commit_timestamp_origin('0'::xid); -- error
+ERROR:  cannot retrieve commit timestamp for transaction 0
+SELECT * FROM pg_xact_commit_timestamp_origin('1'::xid); -- ok, NULL
+ timestamp | origin=20
+-----------+--------
+           |      =20
+(1 row)
+
+SELECT * FROM pg_xact_commit_timestamp_origin('2'::xid); -- ok, NULL
+ timestamp | origin=20
+-----------+--------
+           |      =20
+(1 row)
+
+-- Test transaction without replication origin
+SELECT txid_current() as txid_no_origin \gset
+SELECT x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_=
range, origin
+  FROM pg_last_committed_xact() x;
+ ts_in_range | origin=20
+-------------+--------
+ t           |      0
+(1 row)
+
+SELECT x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_=
range, origin
+  FROM pg_xact_commit_timestamp_origin(:'txid_no_origin') x;
+ ts_in_range | origin=20
+-------------+--------
+ t           |      0
+(1 row)
+
+-- Test transaction with replication origin
+SELECT pg_replication_origin_create('test_commit_ts: get_origin');
+ pg_replication_origin_create=20
+------------------------------
+                            1
+(1 row)
+
+SELECT pg_replication_origin_session_setup('test_commit_ts: get_origin');
+ pg_replication_origin_session_setup=20
+-------------------------------------
+=20
+(1 row)
+
+SELECT txid_current() as txid_with_origin \gset
+SELECT x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_=
range, origin
+  FROM pg_last_committed_xact() x;
+ ts_in_range | origin=20
+-------------+--------
+ t           |      1
+(1 row)
+
+SELECT x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_=
range, origin
+  FROM pg_xact_commit_timestamp_origin(:'txid_with_origin') x;
+ ts_in_range | origin=20
+-------------+--------
+ t           |      1
+(1 row)
+
+SELECT pg_replication_origin_session_reset();
+ pg_replication_origin_session_reset=20
+-------------------------------------
+=20
+(1 row)
+
+SELECT pg_replication_origin_drop('test_commit_ts: get_origin');
+ pg_replication_origin_drop=20
+----------------------------
+=20
 (1 row)
=20
diff --git a/src/test/modules/commit_ts/expected/commit_timestamp_1.out b/s=
rc/test/modules/commit_ts/expected/commit_timestamp_1.out
index c10b0abc2b..34da5b8624 100644
--- a/src/test/modules/commit_ts/expected/commit_timestamp_1.out
+++ b/src/test/modules/commit_ts/expected/commit_timestamp_1.out
@@ -34,6 +34,74 @@ SELECT pg_xact_commit_timestamp('2'::xid);
 =20
 (1 row)
=20
-SELECT x.xid::text::bigint > 0, x.timestamp > '-infinity'::timestamptz, x.=
timestamp <=3D now() FROM pg_last_committed_xact() x;
+SELECT x.xid::text::bigint > 0 as xid_valid,
+    x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_ran=
ge,
+    origin
+  FROM pg_last_committed_xact() x;
 ERROR:  could not get commit timestamp data
 HINT:  Make sure the configuration parameter "track_commit_timestamp" is s=
et.
+-- Test non-normal transaction ids.
+SELECT * FROM pg_xact_commit_timestamp_origin(NULL); -- ok, NULL
+ timestamp | origin=20
+-----------+--------
+           |      =20
+(1 row)
+
+SELECT * FROM pg_xact_commit_timestamp_origin('0'::xid); -- error
+ERROR:  cannot retrieve commit timestamp for transaction 0
+SELECT * FROM pg_xact_commit_timestamp_origin('1'::xid); -- ok, NULL
+ timestamp | origin=20
+-----------+--------
+           |      =20
+(1 row)
+
+SELECT * FROM pg_xact_commit_timestamp_origin('2'::xid); -- ok, NULL
+ timestamp | origin=20
+-----------+--------
+           |      =20
+(1 row)
+
+-- Test transaction without replication origin
+SELECT txid_current() as txid_no_origin \gset
+SELECT x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_=
range, origin
+  FROM pg_last_committed_xact() x;
+ERROR:  could not get commit timestamp data
+HINT:  Make sure the configuration parameter "track_commit_timestamp" is s=
et.
+SELECT x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_=
range, origin
+  FROM pg_xact_commit_timestamp_origin(:'txid_no_origin') x;
+ERROR:  could not get commit timestamp data
+HINT:  Make sure the configuration parameter "track_commit_timestamp" is s=
et.
+-- Test transaction with replication origin
+SELECT pg_replication_origin_create('test_commit_ts: get_origin');
+ pg_replication_origin_create=20
+------------------------------
+                            1
+(1 row)
+
+SELECT pg_replication_origin_session_setup('test_commit_ts: get_origin');
+ pg_replication_origin_session_setup=20
+-------------------------------------
+=20
+(1 row)
+
+SELECT txid_current() as txid_with_origin \gset
+SELECT x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_=
range, origin
+  FROM pg_last_committed_xact() x;
+ERROR:  could not get commit timestamp data
+HINT:  Make sure the configuration parameter "track_commit_timestamp" is s=
et.
+SELECT x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_=
range, origin
+  FROM pg_xact_commit_timestamp_origin(:'txid_with_origin') x;
+ERROR:  could not get commit timestamp data
+HINT:  Make sure the configuration parameter "track_commit_timestamp" is s=
et.
+SELECT pg_replication_origin_session_reset();
+ pg_replication_origin_session_reset=20
+-------------------------------------
+=20
+(1 row)
+
+SELECT pg_replication_origin_drop('test_commit_ts: get_origin');
+ pg_replication_origin_drop=20
+----------------------------
+=20
+(1 row)
+
diff --git a/src/test/modules/commit_ts/sql/commit_timestamp.sql b/src/test=
/modules/commit_ts/sql/commit_timestamp.sql
index 4e041a5347..72390614e7 100644
--- a/src/test/modules/commit_ts/sql/commit_timestamp.sql
+++ b/src/test/modules/commit_ts/sql/commit_timestamp.sql
@@ -21,4 +21,32 @@ SELECT pg_xact_commit_timestamp('0'::xid);
 SELECT pg_xact_commit_timestamp('1'::xid);
 SELECT pg_xact_commit_timestamp('2'::xid);
=20
-SELECT x.xid::text::bigint > 0, x.timestamp > '-infinity'::timestamptz, x.=
timestamp <=3D now() FROM pg_last_committed_xact() x;
+SELECT x.xid::text::bigint > 0 as xid_valid,
+    x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_ran=
ge,
+    origin
+  FROM pg_last_committed_xact() x;
+
+-- Test non-normal transaction ids.
+SELECT * FROM pg_xact_commit_timestamp_origin(NULL); -- ok, NULL
+SELECT * FROM pg_xact_commit_timestamp_origin('0'::xid); -- error
+SELECT * FROM pg_xact_commit_timestamp_origin('1'::xid); -- ok, NULL
+SELECT * FROM pg_xact_commit_timestamp_origin('2'::xid); -- ok, NULL
+
+-- Test transaction without replication origin
+SELECT txid_current() as txid_no_origin \gset
+SELECT x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_=
range, origin
+  FROM pg_last_committed_xact() x;
+SELECT x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_=
range, origin
+  FROM pg_xact_commit_timestamp_origin(:'txid_no_origin') x;
+
+-- Test transaction with replication origin
+SELECT pg_replication_origin_create('test_commit_ts: get_origin');
+SELECT pg_replication_origin_session_setup('test_commit_ts: get_origin');
+SELECT txid_current() as txid_with_origin \gset
+SELECT x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_=
range, origin
+  FROM pg_last_committed_xact() x;
+SELECT x.timestamp <@ tstzrange('-infinity'::timestamptz, now()) AS ts_in_=
range, origin
+  FROM pg_xact_commit_timestamp_origin(:'txid_with_origin') x;
+
+SELECT pg_replication_origin_session_reset();
+SELECT pg_replication_origin_drop('test_commit_ts: get_origin');
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index f065856535..1080d3a666 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23397,6 +23397,21 @@ SELECT collation for ('foo' COLLATE "de_DE");
        </para></entry>
       </row>
=20
+      <row>
+       <entry role=3D"func_table_entry"><para role=3D"func_signature">
+        <indexterm>
+         <primary>pg_xact_commit_timestamp_origin</primary>
+        </indexterm>
+        <function>pg_xact_commit_timestamp_origin</function> ()
+        <returnvalue>record</returnvalue>
+        ( <parameter>timestamp</parameter> <type>timestamp with time zone<=
/type>,
+         <parameter>Oid</parameter> <type>origin</type>)
+       </para>
+       <para>
+         Returns the commit timestamp and replication origin of a transact=
ion.
+       </para></entry>
+      </row>
+
       <row>
        <entry role=3D"func_table_entry"><para role=3D"func_signature">
         <indexterm>
@@ -23405,11 +23420,12 @@ SELECT collation for ('foo' COLLATE "de_DE");
         <function>pg_last_committed_xact</function> ()
         <returnvalue>record</returnvalue>
         ( <parameter>xid</parameter> <type>xid</type>,
-        <parameter>timestamp</parameter> <type>timestamp with time zone</t=
ype> )
+        <parameter>timestamp</parameter> <type>timestamp with time zone</t=
ype>,
+        <parameter>Oid</parameter> <type>origin</type> )
        </para>
        <para>
-        Returns the transaction ID and commit timestamp of the latest
-        committed transaction.
+        Returns the transaction ID, commit timestamp and replication origin
+        of the latest committed transaction.
        </para></entry>
       </row>
      </tbody>

--d6Gm4EdcadzBjdND--

--TakKZr9L6Hm6aLOc
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl8ECbQACgkQnvQgOdby
QH3psQ/7BJtdMIJ6R775mQOMCyXA4mv7V8p/9VIFBI6Cc/lTwfCLr8M0E8nKD1cE
70OjvfqHpfi7Zxl17u5mUJMI9LzGBH5qNyxrGOlZbuWGC+bxd6SMvpIUakgHV4zH
1Ka6FCrJkLQxYJ6OUzFFLViWZ8H85oEKB/Mjt4tzwxSBACYBLifhYIYw+kjWol77
OHHcGighlfJaK+1su96O3qprIdaMNSfyG9yW2ZdzIxW2B7MEYhzqh1gak5MDH1wk
vpY0bGpw9JLcuLivPmajoebOWPZF/hGyTC8wssG6ifYeh6ZYYU1K6vP313EXy/hx
ccooHZLgWo7h6ctaPrCNzbZy4ZFJ5iW4jZS6qUVAEocjLNsQr4HPVKNUlxtoyE4y
UpoYkUKlr64cyIlbZGOzVEmHwfW3o716T0OTEHX17l+FYgq37tzOUCG/G+KBZ1Hk
IxFKhNTrPcMcatskGo2Z8MPEYyijK1Rk/b46DboaZvHLhwTE+/uXt31bXKpXmW3O
m29OuoPenlXJxYnG26kYKEw5tPdFP95CoTKCK6IttigNWMGN4Dn/RV+uppTrzJXs
kplvQgFbvo5Me0HTIo/MALLDdO0uLPBKcpvMLFo7Qng626qMJ6HhtNN1xmXCqqEs
mzj/HalDgDANw+RT+swqMrPooTHmBBlotlFRJ4Vf9hUYyTqe5HM=
=FHQ8
-----END PGP SIGNATURE-----

--TakKZr9L6Hm6aLOc--