A patch for get origin from commit_ts.

Next Topic
 
classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

A patch for get origin from commit_ts.

Movead Li
Hello hackers,

I am researching about 'origin' in PostgreSQL, mainly it used in logical
decoding to filter transaction from non-local source.. I notice that the
'origin' is stored in commit_ts so that I think we are possible to get 'origin'
of a transaction from commit_ts.

But I can not fond any code to get 'origin' from commit_ts, just like it is
producing data which nobody cares about. Can I know what's the purpose
of the 'origin' in commit_ts? Do you think we should add some support
to the careless data?

For example, I add a function to get 'origin' from commit_ts:
=======================================
postgres=# select pg_xact_commit_origin('490');
 pg_xact_commit_origin 
-----------------------
 test_origin
(1 row)

postgres=# select pg_xact_commit_origin('491');
 pg_xact_commit_origin 
-----------------------
 test_origin1
(1 row)

postgres=#
=======================================



Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca

get_origin_from_commit_ts.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: A patch for get origin from commit_ts.

Michael Paquier-2
On Mon, May 11, 2020 at 04:43:11PM +0800, [hidden email] wrote:
> But I can not fond any code to get 'origin' from commit_ts, just like it is
> producing data which nobody cares about. Can I know what's the purpose
> of the 'origin' in commit_ts? Do you think we should add some support
> to the careless data?

I have not thought about this matter, but it seems to me that you
should add this patch to the upcoming commit fest for evaluation:
https://commitfest.postgresql.org/28/

This is going to take a couple of months though as the main focus
lately is the stability of 13.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: A patch for get origin from commit_ts.

Movead Li

>I have not thought about this matter, but it seems to me that you
>should add this patch to the upcoming commit fest for evaluation:
Thanks.

I think about it more detailed, and find it's better to show the 'roident'
other than 'roname'. Because an old 'roident' value will be used
immediately after dropped, and a new patch attached with test case
and documentation.

============================================
SELECT pg_xact_commit_origin('490');
 pg_xact_commit_origin 
-----------------------
                     1
(1 row)

SELECT pg_xact_commit_origin('491');
 pg_xact_commit_origin 
-----------------------
                     2
(1 row)
============================================



Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca

get_origin_from_commit_ts_v2.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: A patch for get origin from commit_ts.

Madan Kumar
Hello hackers,

We already have pg_xact_commit_timestamp() that returns the timestamp of
the commit. It may be better to have one single function returning both
timestamp and origin for a given transaction ID.

A second thing is that TransactionIdGetCommitTsData() was introdued in
core(73c986add). It has only one caller pg_xact_commit_timestamp() which
passes RepOriginId as NULL, making last argument to the
TransactionIdGetCommitTsData() a dead code in core.

Quick code search shows that it is getting used by pglogical (caller:
https://sources.debian.org/src/pglogical/2.3.2-1/pglogical_conflict.c/?hl=509#L509).
CCing Craig Ringer and Petr Jelinek for the inputs.

Warm Regards,
Madan Kumar K
"There is no Elevator to Success. You have to take the Stairs"


Reply | Threaded
Open this post in threaded view
|

Re: A patch for get origin from commit_ts.

Michael Paquier-2
On Mon, Jun 29, 2020 at 06:17:27PM -0700, Madan Kumar wrote:

> We already have pg_xact_commit_timestamp() that returns the timestamp of
> the commit. It may be better to have one single function returning both
> timestamp and origin for a given transaction ID.
>
> A second thing is that TransactionIdGetCommitTsData() was introdued in
> core(73c986add). It has only one caller pg_xact_commit_timestamp() which
> passes RepOriginId as NULL, making last argument to the
> TransactionIdGetCommitTsData() a dead code in core.
>
> Quick code search shows that it is getting used by pglogical (caller:
> https://sources.debian.org/src/pglogical/2.3.2-1/pglogical_conflict.c/?hl=509#L509).
> CCing Craig Ringer and Petr Jelinek for the inputs.
Another question that has popped up when doing this review is what
would be the use-case of adding this information at SQL level knowing
that logical replication exists since 10?  Having dead code in the
backend tree is not a good idea of course, so we can also have as
argument to simplify TransactionIdGetCommitTsData().  Now, pglogical
has pglogical_xact_commit_timestamp_origin() to get the replication
origin with its own function so providing an extending equivalent
returning one row with two fields would be nice for pglogical so as
this function is not necessary.  As mentioned by Madan, the portion of
the code using TransactionIdGetCommitTsData() relies on it for
conflicts of updates (the first win, last win logic at quick glance).

I am adding Peter E in CC for an opinion, the last commits of
pglogical are from him.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: A patch for get origin from commit_ts.

Movead Li

>> A second thing is that TransactionIdGetCommitTsData() was introdued in
>> core(73c986add). It has only one caller pg_xact_commit_timestamp() which
>> passes RepOriginId as NULL, making last argument to the
>> TransactionIdGetCommitTsData() a dead code in core.
>>
>> Quick code search shows that it is getting used by pglogical (caller:
>> https://sources.debian.org/src/pglogical/2.3.2-1/pglogical_conflict.c/?hl=509#L509).
>> CCing Craig Ringer and Petr Jelinek for the inputs.
 
>Another question that has popped up when doing this review is what
>would be the use-case of adding this information at SQL level knowing
>that logical replication exists since 10?  Having dead code in the
>backend tree is not a good idea of course, so we can also have as
>argument to simplify TransactionIdGetCommitTsData().  Now, pglogical
>has pglogical_xact_commit_timestamp_origin() to get the replication
>origin with its own function so providing an extending equivalent
>returning one row with two fields would be nice for pglogical so as
>this function is not necessary.  As mentioned by Madan, the portion of
>the code using TransactionIdGetCommitTsData() relies on it for
>conflicts of updates (the first win, last win logic at quick glance).

Thanks for the explanation, the origin in commit_ts seems useless, I am just
want to know why it appears there. It's ok to close this issue if we do not
want to touch it now.

And I am more interest in origin in wal, if data from a logical replicate or a 
manual origin then many wal records will get a 'RepOriginId',  'RepOriginId'
in 'xact' wal record may help to do some filter, the other same dead code
too. So can you help me to understand why or the historical reason for that?



Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca
Reply | Threaded
Open this post in threaded view
|

Re: A patch for get origin from commit_ts.

Simon Riggs
In reply to this post by Madan Kumar
On Tue, 30 Jun 2020 at 02:17, Madan Kumar <[hidden email]> wrote:
 
We already have pg_xact_commit_timestamp() that returns the timestamp of
the commit.

Yes, pg_xact_commit_origin() is a good name for an additional function. +1 for this.
 
It may be better to have one single function returning both
timestamp and origin for a given transaction ID.

No need to change existing APIs.

--
Simon Riggs                http://www.2ndQuadrant.com/
Mission Critical Databases
Reply | Threaded
Open this post in threaded view
|

Re: A patch for get origin from commit_ts.

Alvaro Herrera-9
In reply to this post by Michael Paquier-2
On 2020-Jun-30, Michael Paquier wrote:

> Another question that has popped up when doing this review is what
> would be the use-case of adding this information at SQL level knowing
> that logical replication exists since 10?

Logical replication in core is a far cry from a fully featured
replication solution.  Kindly do not claim that we can now remove
features just because in-core logical replication does not use them;
this argument is ignoring the fact that we're still a long way from
developing actually powerful logical replication capabilities.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: A patch for get origin from commit_ts.

Michael Paquier-2
On Tue, Jun 30, 2020 at 02:32:47PM -0400, Alvaro Herrera wrote:

> On 2020-Jun-30, Michael Paquier wrote:
>> Another question that has popped up when doing this review is what
>> would be the use-case of adding this information at SQL level knowing
>> that logical replication exists since 10?
>
> Logical replication in core is a far cry from a fully featured
> replication solution.  Kindly do not claim that we can now remove
> features just because in-core logical replication does not use them;
> this argument is ignoring the fact that we're still a long way from
> developing actually powerful logical replication capabilities.
Thanks for the feedback.  If that sounded aggressive in some way, this
was not my intention, so my apologies for that.  Now, I have to admit
that I am worried to see in core code that stands as dead without any
actual way to test it directly.  Somebody hacking this code cannot be
sure if they are breaking it or not, except if they test it with
pglogical.  So it is good to close the gap here.  It also brings a
second point IMO, could the documentation be improved to describe more
use-cases where these functions would be useful?  The documentation
gap is not a problem this patch has to deal with, though.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: A patch for get origin from commit_ts.

Michael Paquier-2
In reply to this post by Simon Riggs
On Tue, Jun 30, 2020 at 01:58:17PM +0100, Simon Riggs wrote:
> On Tue, 30 Jun 2020 at 02:17, Madan Kumar <[hidden email]> wrote:
>> It may be better to have one single function returning both
>> timestamp and origin for a given transaction ID.
>
> No need to change existing APIs.

Adding a new function able to return both fields at the same time does
not imply that we'd remove the original one, it just implies that we
would be able to retrieve both fields with a single call of
TransactionIdGetCommitTsData(), saving from an extra CommitTsSLRULock
taken, etc.  That's actually what pglogical does with
its pglogical_xact_commit_timestamp_origin() in
pglogical_functions.c.  So adding one function able to return one
tuple with the two fields, without removing the existing
pg_xact_commit_timestamp() makes the most sense, no?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: A patch for get origin from commit_ts.

Simon Riggs
On Thu, 2 Jul 2020 at 02:58, <[hidden email]> wrote:
On Tue, Jun 30, 2020 at 01:58:17PM +0100, Simon Riggs wrote:
> On Tue, 30 Jun 2020 at 02:17, Madan Kumar <[hidden email]> wrote:
>> It may be better to have one single function returning both
>> timestamp and origin for a given transaction ID.
>
> No need to change existing APIs.

Adding a new function able to return both fields at the same time does
not imply that we'd remove the original one, it just implies that we
would be able to retrieve both fields with a single call of
TransactionIdGetCommitTsData(), saving from an extra CommitTsSLRULock
taken, etc.  That's actually what pglogical does with
its pglogical_xact_commit_timestamp_origin() in
pglogical_functions.c.  So adding one function able to return one
tuple with the two fields, without removing the existing
pg_xact_commit_timestamp() makes the most sense, no?

OK 

--
Simon Riggs                http://www.2ndQuadrant.com/
Mission Critical Databases
Reply | Threaded
Open this post in threaded view
|

Re: A patch for get origin from commit_ts.

Petr Jelinek
In reply to this post by Michael Paquier-2
On 02/07/2020 03:58, [hidden email] wrote:

> On Tue, Jun 30, 2020 at 01:58:17PM +0100, Simon Riggs wrote:
>> On Tue, 30 Jun 2020 at 02:17, Madan Kumar <[hidden email]> wrote:
>>> It may be better to have one single function returning both
>>> timestamp and origin for a given transaction ID.
>>
>> No need to change existing APIs.
>
> Adding a new function able to return both fields at the same time does
> not imply that we'd remove the original one, it just implies that we
> would be able to retrieve both fields with a single call of
> TransactionIdGetCommitTsData(), saving from an extra CommitTsSLRULock
> taken, etc.  That's actually what pglogical does with
> its pglogical_xact_commit_timestamp_origin() in
> pglogical_functions.c.  So adding one function able to return one
> tuple with the two fields, without removing the existing
> pg_xact_commit_timestamp() makes the most sense, no?


Agreed, sounds reasonable.

I also (I suspect like Álvaro) parsed your original message as wanting
to remove origin from the record completely.

--
Petr Jelinek
2ndQuadrant - PostgreSQL Solutions for the Enterprise
https://www.2ndQuadrant.com/


Reply | Threaded
Open this post in threaded view
|

Re: A patch for get origin from commit_ts.

Michael Paquier-2
On Thu, Jul 02, 2020 at 10:12:02AM +0200, Petr Jelinek wrote:

> On 02/07/2020 03:58, [hidden email] wrote:
>> Adding a new function able to return both fields at the same time does
>> not imply that we'd remove the original one, it just implies that we
>> would be able to retrieve both fields with a single call of
>> TransactionIdGetCommitTsData(), saving from an extra CommitTsSLRULock
>> taken, etc.  That's actually what pglogical does with
>> its pglogical_xact_commit_timestamp_origin() in
>> pglogical_functions.c.  So adding one function able to return one
>> tuple with the two fields, without removing the existing
>> pg_xact_commit_timestamp() makes the most sense, no?
>
> Agreed, sounds reasonable.
Thanks.  Movead, please note that the patch is waiting on author?
Could you send an update if you think that those changes make sense?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: A patch for get origin from commit_ts.

Movead Li


>Thanks. Movead, please note that the patch is waiting on author?
>Could you send an update if you think that those changes make sense?
Thanks for approval the issue, I will send a patch at Monday. 
Regards,
Highgo Software (Canada/China/Pakistan)
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca


Reply | Threaded
Open this post in threaded view
|

Re: A patch for get origin from commit_ts.

Movead Li
In reply to this post by Michael Paquier-2
>Thanks.  Movead, please note that the patch is waiting on author?
>Could you send an update if you think that those changes make sense?

I make a patch as Michael Paquier described that use a new function to
return transactionid and origin, and I add a origin version to 
pg_last_committed_xact() too,  now it looks like below:

============================================
postgres=# SELECT txid_current() as txid \gset
postgres=# SELECT * FROM  pg_xact_commit_timestamp_origin(:'txid');
           timestamp                           |   origin 
-------------------------------------+--------
 2020-07-04 17:52:10.199623+08 |      1
(1 row)

postgres=# SELECT * FROM pg_last_committed_xact_with_origin();
 xid  |           timestamp                          | origin 
-----+------------------------------------+--------
 506 | 2020-07-04 17:52:10.199623+08 |      1
(1 row)

postgres=#
============================================

---
Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca

get_origin_from_commit_ts_v3.patch (20K) Download Attachment