Re: tracking commit timestamps

Hi,

There has been some interest in keeping track of timestamp of
transaction commits. This patch implements that.

There are some seemingly curious choices here. First, this module can
be disabled, and in fact it's turned off by default. At startup, we
verify whether it's enabled, and create the necessary SLRU segments if
so. And if the server is started with this disabled, we set the oldest
value we know about to avoid trying to read the commit TS of
transactions of which we didn't keep record. The ability to turn this
off is there to avoid imposing the overhead on systems that don't need
this feature.

Another thing of note is that we allow for some extra data alongside the
timestamp proper. This might be useful for a replication system that
wants to keep track of the origin node ID of a committed transaction,
for example. Exactly what will we do with the bit space we have is
unclear, so I have kept it generic and called it "commit extra data".

This offers the chance for outside modules to set the commit TS of a
transaction; there is support for WAL-logging such values. But the core
user of the feature (RecordTransactionCommit) doesn't use it, because
xact.c's WAL logging itself is enough. For systems that are replicating
transactions from remote nodes, it is useful.

We also keep track of the latest committed transaction. This is
supposed to be useful to calculate replication lag.

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

On Wed, Oct 23, 2013 at 3:46 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Hi,
>
> There has been some interest in keeping track of timestamp of
> transaction commits. This patch implements that.

Some of the use cases, I could think of are
1. Is it for usecases such that if user want to read all data of table
where transaction commit_ts <= '2012-04-04 09:30:00'?
2. for replication systems, may be the middleware can use it to replay
transactions in some remote system.
3. Is there any use of this feature in logical-rep/change data extraction?

> There are some seemingly curious choices here. First, this module can
> be disabled, and in fact it's turned off by default. At startup, we
> verify whether it's enabled, and create the necessary SLRU segments if
> so. And if the server is started with this disabled, we set the oldest
> value we know about to avoid trying to read the commit TS of
> transactions of which we didn't keep record. The ability to turn this
> off is there to avoid imposing the overhead on systems that don't need
> this feature.
>
> Another thing of note is that we allow for some extra data alongside the
> timestamp proper. This might be useful for a replication system that
> wants to keep track of the origin node ID of a committed transaction,
> for example. Exactly what will we do with the bit space we have is
> unclear, so I have kept it generic and called it "commit extra data".

"commit extra data" can be LSN of commit log record, but I think it
will also depend on how someone wants to use this feature.
To suggest for storing LSN, I had referred information at below page
which describes about similar information for each transaction.
http://technet.microsoft.com/en-us/library/cc645959.aspx

> This offers the chance for outside modules to set the commit TS of a
> transaction; there is support for WAL-logging such values. But the core
> user of the feature (RecordTransactionCommit) doesn't use it, because
> xact.c's WAL logging itself is enough.

I have one question for the case when commits is set from
RecordTransactionCommit().

*** 1118,1123 **** RecordTransactionCommit(void)
--- 1119,1132 ----
}

/*
+ * We don't need to log the commit timestamp separately since the commit
+ * record logged above has all the necessary action to set the timestamp
+ * again.
+ */
+ TransactionTreeSetCommitTimestamp(xid, nchildren, children,
+ xactStopTimestamp, 0, false);
+

Here for CLOG, we are doing Xlogflush before writing to Clog page, but
Committs writes timestamp before XlogFlush().
Won't that create problem for synchronous commit as Checkpoint can
takecare of flushing Xlog for relation pages before flush of page,
but for Clog/Committs RecordTransactionCommit() should take care of doing it.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Oct 22, 2013 at 5:16 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Hi,
>
> There has been some interest in keeping track of timestamp of
> transaction commits. This patch implements that.
>

Hi,

Sorry for the delay on the review.

First, because of the recent fixes the patch doesn't apply cleanly
anymore but the changes seems to be easy.

=== performance ===

i expected a regression on performance with the module turned on
because of the new XLOG records and wrote of files in pg_committs but
the performance drop is excessive.

Master 437.835674 tps
Patch, guc off 436.4340982 tps
Patch, guc on 0.370524 tps

This is in a pgbench's db initialized with scale=50 and run with
"pgbench -c 64 -j 16 -n -T 300" 5 times (values above are the average
of runs)

postgresql changes:

shared_buffers = 1GB
full_page_writes = off
checkpoint_segments = 30
checkpoint_timeout = 15min
random_page_cost = 2.0

== functionality ==

I started the server with the module off, then after some work turned
the module on and restarted the server and run a few benchs then
turned it off again and restart the server and get a message like:

"""
LOG: database system was not properly shut down; automatic recovery in progress
LOG: record with zero length at 0/3112AE58
LOG: redo is not required
FATAL: cannot make new WAL entries during recovery
LOG: startup process (PID 24876) exited with exit code 1
LOG: aborting startup due to startup process failure
"""

--
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157

On 2013-12-02 02:39:55 -0500, Jaime Casanova wrote:
> === performance ===
>
> i expected a regression on performance with the module turned on
> because of the new XLOG records and wrote of files in pg_committs but
> the performance drop is excessive.
> Master 437.835674 tps
> Patch, guc off 436.4340982 tps
> Patch, guc on 0.370524 tps

There clearly is something wrong. The additional amount of xlog records
should be nearly unnoticeable because committs piggybacks on commit
records.

> I started the server with the module off, then after some work turned
> the module on and restarted the server and run a few benchs then
> turned it off again and restart the server and get a message like:
>
> """
> LOG: database system was not properly shut down; automatic recovery in progress
> LOG: record with zero length at 0/3112AE58
> LOG: redo is not required
> FATAL: cannot make new WAL entries during recovery
> LOG: startup process (PID 24876) exited with exit code 1
> LOG: aborting startup due to startup process failure
> """

Alvaro: That's because of the location you call StartupCommitts - a)
it's called at the beginning of recovery if HS is enabled b) it's called
before StartupXLOG() does LocalSetXLogInsertAllowed().

So I think you need to split StartupCommitts into StartupCommitts() and
TrimCommitts() where only the latter does the trimming if committs is
disabled.
I also wonder if track_commit_timestamp should be tracked by the the
XLOG_PARAMETER_CHANGE stuff? So it's not disabled on the standby when
it's enabled on the primary?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 10/23/2013 01:16 AM, Alvaro Herrera wrote:
> There has been some interest in keeping track of timestamp of
> transaction commits. This patch implements that.
>
> There are some seemingly curious choices here. First, this module can
> be disabled, and in fact it's turned off by default. At startup, we
> verify whether it's enabled, and create the necessary SLRU segments if
> so. And if the server is started with this disabled, we set the oldest
> value we know about to avoid trying to read the commit TS of
> transactions of which we didn't keep record. The ability to turn this
> off is there to avoid imposing the overhead on systems that don't need
> this feature.
>
> Another thing of note is that we allow for some extra data alongside the
> timestamp proper. This might be useful for a replication system that
> wants to keep track of the origin node ID of a committed transaction,
> for example. Exactly what will we do with the bit space we have is
> unclear, so I have kept it generic and called it "commit extra data".
>
> This offers the chance for outside modules to set the commit TS of a
> transaction; there is support for WAL-logging such values. But the core
> user of the feature (RecordTransactionCommit) doesn't use it, because
> xact.c's WAL logging itself is enough. For systems that are replicating
> transactions from remote nodes, it is useful.
>
> We also keep track of the latest committed transaction. This is
> supposed to be useful to calculate replication lag.

Generally speaking, I'm not in favor of adding dead code, even if it
might be useful to someone in the future. For one, it's going to get
zero testing. Once someone comes up with an actual use case, let's add
that stuff at that point. Otherwise there's a good chance that we build
something that's almost but not quite useful.

Speaking of the functionality this does offer, it seems pretty limited.
A commit timestamp is nice, but it isn't very interesting on its own.
You really also want to know what the transaction did, who ran it, etc.
ISTM some kind of a auditing or log-parsing system that could tell you
all that would be much more useful, but this patch doesn't get us any
closer to that.

Does this handle XID wraparound correctly? SLRU has a maximum of 64k
segments with 32 SLRU pages each. With 12 bytes per each commit entry,
that's not enough to hold the timestamp and "commit extra data" of the
whole 2^31 XID range: (8192 * 32 * 65536) / 12 = 1431655765. And that's
with the default page size, with smaller pages you run into the limit
quicker.

It would be nice to teach SLRU machinery how to deal with more than 64k
segments. SSI code in twophase.c ran into the same limit, and all you
get is a warning there.

- Heikki

On 2013-12-10 11:56:45 +0200, Heikki Linnakangas wrote:
> Speaking of the functionality this does offer, it seems pretty limited. A
> commit timestamp is nice, but it isn't very interesting on its own. You
> really also want to know what the transaction did, who ran it, etc. ISTM
> some kind of a auditing or log-parsing system that could tell you all that
> would be much more useful, but this patch doesn't get us any closer to
> that.

It's useful for last-update-wins for async multimaster. Currently
several userspace solutions try to approximate it by inserting a
timestamps into a column when a row is inserted or updated, but that is
quite limiting because either the number is out of date wrt. the commit
and/or it will differ between the rows.

I don't see how you could get an accurate timestamp in a significantly
different way?

> Does this handle XID wraparound correctly? SLRU has a maximum of 64k
> segments with 32 SLRU pages each. With 12 bytes per each commit entry,
> that's not enough to hold the timestamp and "commit extra data" of the whole
> 2^31 XID range: (8192 * 32 * 65536) / 12 = 1431655765. And that's with the
> default page size, with smaller pages you run into the limit quicker.
>
> It would be nice to teach SLRU machinery how to deal with more than 64k
> segments. SSI code in twophase.c ran into the same limit, and all you get is
> a warning there.

Yea, 9.3 is already running afoul of that, because of the changed size
for the multixact member pages. Came up just yesterday in the course of
#8673.

(gdb) p/x (1L<<32)/(MULTIXACT_MEMBERS_PER_PAGE * SLRU_PAGES_PER_SEGMENT)
$10 = 0x14078

Is this limitation actually documented anywhere? And is there anything
that needs to be changed but SlruScanDirectory()?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Dec 10, 2013 at 4:56 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> Generally speaking, I'm not in favor of adding dead code, even if it might
> be useful to someone in the future. For one, it's going to get zero testing.
> Once someone comes up with an actual use case, let's add that stuff at that
> point. Otherwise there's a good chance that we build something that's almost
> but not quite useful.

Fair.

> Speaking of the functionality this does offer, it seems pretty limited. A
> commit timestamp is nice, but it isn't very interesting on its own. You
> really also want to know what the transaction did, who ran it, etc. ISTM
> some kind of a auditing or log-parsing system that could tell you all that
> would be much more useful, but this patch doesn't get us any closer to that.

For what it's worth, I think that this has been requested numerous
times over the years by numerous developers of replication solutions.
My main question (apart from whether or not it may have bugs) is
whether it makes a noticeable performance difference. If it does,
that sucks. If it does not, maybe we ought to enable it by default.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas escribió:
> On Tue, Dec 10, 2013 at 4:56 AM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:

> > Speaking of the functionality this does offer, it seems pretty limited. A
> > commit timestamp is nice, but it isn't very interesting on its own. You
> > really also want to know what the transaction did, who ran it, etc. ISTM
> > some kind of a auditing or log-parsing system that could tell you all that
> > would be much more useful, but this patch doesn't get us any closer to that.
>
> For what it's worth, I think that this has been requested numerous
> times over the years by numerous developers of replication solutions.
> My main question (apart from whether or not it may have bugs) is
> whether it makes a noticeable performance difference. If it does,
> that sucks. If it does not, maybe we ought to enable it by default.

I expect it will have some performance impact -- this is why we made it
disable-able in the first place, and why I went to the trouble of
ensuring it can be turned on after initdb. Normal pg_clog entries are 2
bits per transaction, whereas the commit timestamp stuff adds 12 *bytes*
per transaction. Not something to be taken lightly, hence it's off by
default. Presumably people who is using one of those replication
systems is okay with taking some (reasonable) performance hit.

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

On Tue, Dec 10, 2013 at 4:04 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Robert Haas escribió:
>> On Tue, Dec 10, 2013 at 4:56 AM, Heikki Linnakangas
>> <hlinnakangas(at)vmware(dot)com> wrote:
>> > Speaking of the functionality this does offer, it seems pretty limited. A
>> > commit timestamp is nice, but it isn't very interesting on its own. You
>> > really also want to know what the transaction did, who ran it, etc. ISTM
>> > some kind of a auditing or log-parsing system that could tell you all that
>> > would be much more useful, but this patch doesn't get us any closer to that.
>>
>> For what it's worth, I think that this has been requested numerous
>> times over the years by numerous developers of replication solutions.
>> My main question (apart from whether or not it may have bugs) is
>> whether it makes a noticeable performance difference. If it does,
>> that sucks. If it does not, maybe we ought to enable it by default.
>
> I expect it will have some performance impact -- this is why we made it
> disable-able in the first place, and why I went to the trouble of
> ensuring it can be turned on after initdb. Normal pg_clog entries are 2
> bits per transaction, whereas the commit timestamp stuff adds 12 *bytes*
> per transaction. Not something to be taken lightly, hence it's off by
> default. Presumably people who is using one of those replication
> systems is okay with taking some (reasonable) performance hit.

Well, writing 12 extra bytes (why not 8?) on each commit is not
intrinsically that expensive. The (poor) design of SLRU might make it
expensive, though, because since it has no fsync absorption queue, so
sometimes you end up waiting for an fsync, and doing that 48x more
often will indeed have some cost. :-(

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

I worked bit on this patch to make it closer to committable state.

There are several bugs fixed, including ones mentioned by Jamie (writing
WAL during recovery).

Also support for pg_resetxlog/pg_upgrade has been implemented by Andres.

I added simple regression test and regression contrib module to cover
both off and on settings.

The SLRU issue Heikki mentioned should be also gone mainly thanks to
638cf09e7 (I did test it too).

One notable thing missing is documentation for the three SQL level
interfaces provided, I plan to add that soon.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 09/09/14 19:05, Petr Jelinek wrote:
> Hi,
>
> I worked bit on this patch to make it closer to committable state.
>
> There are several bugs fixed, including ones mentioned by Jamie (writing
> WAL during recovery).
>
> Also support for pg_resetxlog/pg_upgrade has been implemented by Andres.
>
> I added simple regression test and regression contrib module to cover
> both off and on settings.
>
> The SLRU issue Heikki mentioned should be also gone mainly thanks to
> 638cf09e7 (I did test it too).
>

Here is updated version that works with current HEAD for the October
committfest.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 13 October 2014 10:05, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:

>> I worked bit on this patch to make it closer to committable state.

> Here is updated version that works with current HEAD for the October
> committfest.

I've reviewed this and it looks good to me. Clean, follows existing
code neatly, documented and tested.

Please could you document a few things

* ExtendCommitTS() works only because commit_ts_enabled can only be
set at server start.
We need that documented so somebody doesn't make it more easily
enabled and break something.
(Could we make it enabled at next checkpoint or similar?)

* The SLRU tracks timestamps of both xids and subxids. We need to
document that it does this because Subtrans SLRU is not persistent. If
we made Subtrans persistent we might need to store only the top level
xid's commitTS, but that's very useful for typical use cases and
wouldn't save much time at commit.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Oct 28, 2014 at 9:25 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> On 13 October 2014 10:05, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>
> >> I worked bit on this patch to make it closer to committable state.
>
> > Here is updated version that works with current HEAD for the October
> > committfest.
>
> I've reviewed this and it looks good to me. Clean, follows existing
> code neatly, documented and tested.
>
> Please could you document a few things
>
> * ExtendCommitTS() works only because commit_ts_enabled can only be
> set at server start.
> We need that documented so somebody doesn't make it more easily
> enabled and break something.
> (Could we make it enabled at next checkpoint or similar?)
>
> * The SLRU tracks timestamps of both xids and subxids. We need to
> document that it does this because Subtrans SLRU is not persistent. If
> we made Subtrans persistent we might need to store only the top level
> xid's commitTS, but that's very useful for typical use cases and
> wouldn't save much time at commit.
>

Hm. What is the performance impact of this feature using the latest version
of this patch? I imagine that the penalty of the additional operations this
feature introduces is not zero, particularly for loads with lots of short
transactions.
--
Michael

On 2014-10-31 14:55:11 +0900, Michael Paquier wrote:
> On Tue, Oct 28, 2014 at 9:25 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>
> > On 13 October 2014 10:05, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> >
> > >> I worked bit on this patch to make it closer to committable state.
> >
> > > Here is updated version that works with current HEAD for the October
> > > committfest.
> >
> > I've reviewed this and it looks good to me. Clean, follows existing
> > code neatly, documented and tested.
> >
> > Please could you document a few things
> >
> > * ExtendCommitTS() works only because commit_ts_enabled can only be
> > set at server start.
> > We need that documented so somebody doesn't make it more easily
> > enabled and break something.
> > (Could we make it enabled at next checkpoint or similar?)
> >
> > * The SLRU tracks timestamps of both xids and subxids. We need to
> > document that it does this because Subtrans SLRU is not persistent. If
> > we made Subtrans persistent we might need to store only the top level
> > xid's commitTS, but that's very useful for typical use cases and
> > wouldn't save much time at commit.
> >
>
> Hm. What is the performance impact of this feature using the latest version
> of this patch?

I haven't measured it recently, but it wasn't large, but measureable.

> I imagine that the penalty of the additional operations this
> feature introduces is not zero, particularly for loads with lots of short
> transactions.

Which is why you can disable it...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Dec 10, 2013 at 2:48 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Speaking of the functionality this does offer, it seems pretty limited. A
>> commit timestamp is nice, but it isn't very interesting on its own. You
>> really also want to know what the transaction did, who ran it, etc. ISTM
>> some kind of a auditing or log-parsing system that could tell you all that
>> would be much more useful, but this patch doesn't get us any closer to that.
>
> For what it's worth, I think that this has been requested numerous
> times over the years by numerous developers of replication solutions.
> My main question (apart from whether or not it may have bugs) is
> whether it makes a noticeable performance difference. If it does,
> that sucks. If it does not, maybe we ought to enable it by default.

+1

It's also requested now and then in the context of auditing and
forensic analysis of application problems. But I also agree that the
tolerance for performance overhead is got to be quite low. If a GUC
is introduced to manage the tradeoff, it should be defaulted to 'on'.

merlin

Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> It's also requested now and then in the context of auditing and
> forensic analysis of application problems. But I also agree that the
> tolerance for performance overhead is got to be quite low. If a GUC
> is introduced to manage the tradeoff, it should be defaulted to 'on'.

Alvaro's original submission specified that the feature defaults to "off".
Since there's no use-case for it unless you've installed some third-party
code (eg an external replication solution), I think that should stay true.
The feature's overhead might be small, but it is most certainly not zero,
and people shouldn't be paying for it unless they need it.

regards, tom lane

On 31/10/14 15:07, Tom Lane wrote:
> Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
>> It's also requested now and then in the context of auditing and
>> forensic analysis of application problems. But I also agree that the
>> tolerance for performance overhead is got to be quite low. If a GUC
>> is introduced to manage the tradeoff, it should be defaulted to 'on'.
>
> Alvaro's original submission specified that the feature defaults to "off".
> Since there's no use-case for it unless you've installed some third-party
> code (eg an external replication solution), I think that should stay true.
> The feature's overhead might be small, but it is most certainly not zero,
> and people shouldn't be paying for it unless they need it.
>

Agreed, that's why it stayed 'off' in my version too.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi,

On 28/10/14 13:25, Simon Riggs wrote:
> On 13 October 2014 10:05, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>
>>> I worked bit on this patch to make it closer to committable state.
>
>> Here is updated version that works with current HEAD for the October
>> committfest.
>
> I've reviewed this and it looks good to me. Clean, follows existing
> code neatly, documented and tested.
>

Thanks for looking at this.

> Please could you document a few things
>
> * ExtendCommitTS() works only because commit_ts_enabled can only be
> set at server start.
> We need that documented so somebody doesn't make it more easily
> enabled and break something.
> (Could we make it enabled at next checkpoint or similar?)

Maybe we could, but that means some kind of two step enabling facility
and I don't want to write that as part of the initial patch since that
will need separate discussion (i.e. do we want to have new GUC flag for
this, or hack solution just for committs?). So maybe later in a
follow-up patch.

> * The SLRU tracks timestamps of both xids and subxids. We need to
> document that it does this because Subtrans SLRU is not persistent. If
> we made Subtrans persistent we might need to store only the top level
> xid's commitTS, but that's very useful for typical use cases and
> wouldn't save much time at commit.

Attached version with the above comments near the relevant code.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 31 October 2014 15:46, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:

> Attached version with the above comments near the relevant code.

Looks cooked and ready to serve. Who's gonna commit this? Alvaro, or
do you want me to?

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Nov 1, 2014 at 1:15 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> On 31 October 2014 15:46, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>
> > Attached version with the above comments near the relevant code.
>
> Looks cooked and ready to serve. Who's gonna commit this? Alvaro, or
> do you want me to?
>
Could you hold on a bit? I'd like to have a look at it more deeply and by
looking at quickly at the code there are a couple of things that could be
improved.
Regards,
--
Michael

On Sat, Nov 1, 2014 at 12:46 AM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:

> Hi,
>
> On 28/10/14 13:25, Simon Riggs wrote:
>
>> On 13 October 2014 10:05, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>>
>> I worked bit on this patch to make it closer to committable state.
>>>>
>>>
>> Here is updated version that works with current HEAD for the October
>>> committfest.
>>>
>>
>> I've reviewed this and it looks good to me. Clean, follows existing
>> code neatly, documented and tested.
>>
>>
> Thanks for looking at this.
>
> Please could you document a few things
>>
>> * ExtendCommitTS() works only because commit_ts_enabled can only be
>> set at server start.
>> We need that documented so somebody doesn't make it more easily
>> enabled and break something.
>> (Could we make it enabled at next checkpoint or similar?)
>>
>
> Maybe we could, but that means some kind of two step enabling facility and
> I don't want to write that as part of the initial patch since that will
> need separate discussion (i.e. do we want to have new GUC flag for this, or
> hack solution just for committs?). So maybe later in a follow-up patch.
>
> * The SLRU tracks timestamps of both xids and subxids. We need to
>> document that it does this because Subtrans SLRU is not persistent. If
>> we made Subtrans persistent we might need to store only the top level
>> xid's commitTS, but that's very useful for typical use cases and
>> wouldn't save much time at commit.
>>
>
> Attached version with the above comments near the relevant code.
>
On a personal note, I think that this is a useful feature, particularly
useful for replication solutions to resolve commit conflicts by using the
method of the first-transaction-that-commits-wins, but this has already
been mentioned on this thread. So yes I am a fan of it, and yes let's keep
the GUC controlling it at off by default.

Now here are a couple of comments at code level, this code seems not enough
baked for a commit:
1) The following renaming should be done:
- pg_get_transaction_committime to pg_get_transaction_commit_time
- pg_get_transaction_extradata to pg_get_transaction_extra_data
- pg_get_transaction_committime_data to pg_get_transaction_commit_time_data
- pg_get_latest_transaction_committime_data to
pg_get_latest_transaction_commit_time_data
2) This patch adds a new option -c in pg_resetxlog to set the transaction
XID of the transaction from which can be retrieved a commit timestamp, but
the documentation of pg_resetxlog is not updated.
3) General remark: committs is not a name suited IMO (ts for
transaction??). What if the code is changed to use commit_time instead?
This remark counts as well for the file names committs.c and committs.h,
and for pg_committs.
4) Nitpicky remark in pg_resetxlog, let's try to respect the alphabetical
order (not completely related to this patch), so not:
+ while ((c = getopt(argc, argv, "D:fl:m:no:O:x:e:c:")) != -1)
but:
+ while ((c = getopt(argc, argv, "c:e:D:fl:m:no:O:x:")) != -1)
5) --help message should be reworked (alphabetical order of the entries),
this will avoid some cycles of Peter as he usually spends time revisiting
and cleaning up such things:
printf(_(" -x XID set next transaction ID\n"));
+ printf(_(" -c XID set the oldest retrievable commit
timestamp\n"));
printf(_(" -?, --help show this help, then exit\n"));
6) To be consistent with everything, shouldn't track_commit_timestamp be
renamed to track_commit_time
7) This documentation portion should be reworked from that:
+ <para>
+ Record commit time of transactions. This parameter
+ can only be set in
+ the <filename>postgresql.conf</> file or on the server command
line.
+ The default value is off.
+ </para>
To roughly that (not the rewording and the use of <literal>):
+ <para>
+ Record commit time of transactions. This parameter
+ can only be set in <filename>postgresql.conf</> or on the server
command line.
+ The default value is <literal>off</literal>.
+ </para>
8) Let's update this file list more consistently:
-OBJS = clogdesc.o dbasedesc.o gindesc.o gistdesc.o hashdesc.o heapdesc.o \
+OBJS = clogdesc.o committsdesc.o dbasedesc.o gindesc.o gistdesc.o
hashdesc.o \
+ heapdesc.o \
mxactdesc.o nbtdesc.o relmapdesc.o seqdesc.o smgrdesc.o
spgdesc.o \
standbydesc.o tblspcdesc.o xactdesc.o xlogdesc.o
9) Hm?! "oldest commit time xid", no?
- "oldest running xid %u;
%s",
+ "oldest CommitTs xid: %u;
oldest running xid %u; %s",
10) I don't see why this diff is in the patch:
/*
- * See how many subxids, if any, are on the same page as the
parent, if
- * any.
+ * See how many subxids, if any, are on the same page as the parent.
*/
11) contrib/Makefile has not been updated with the new module test_committs
that this patch introduces.
12) In committs_desc(at)committsdesc(dot)c, isn't this block overthinking a bit:
+ else
+ appendStringInfo(buf, "UNKNOWN");
It may be better to remove it, no?
13) Isn't that 2014?
+ * Portions Copyright (c) 1996-2013, PostgreSQL Global Development Group
14) I'd put the two checks in the reverse order:
+ Assert(xid != InvalidTransactionId);
+
+ if (!commit_ts_enabled)
+ return;
15) The multiple calls to PG_FUNCTION_INFO_V1 in committs.c are not
necessary. Those functions are already defined in pg_proc.h.
16) make installcheck (test committs_off) fails on a server that has
track_committs set to on. You should use an alternate output. I would
recommend removing as well the _off suffix in the test name. Let's use
commit_time. Also, it should be mentioned in parallel_schedule with a
comment that this test should always run alone and never in parallel with
other tests. Honestly, I also think that test_committs brings no additional
value and results in duplication code between src/test/regress and
contrib/test_committs. So I'd just rip it off. On top of that, I think that
"SHOW track_committs" should be added in the list of commands run in the
test. We actually want to check of commit time are really registered if the
feature switch is on or off.

I am still planning to do more extensive tests, and study a bit more
committs.c (with even more comments) as it is the core part of the feature.
For now I'd recommend to hold on commit fire for this patch.
Regards,
--
Michael

Hi,

thanks for review.

On 01/11/14 05:45, Michael Paquier wrote:
> Now here are a couple of comments at code level, this code seems not
> enough baked for a commit:
> 1) The following renaming should be done:
> - pg_get_transaction_committime to pg_get_transaction_commit_time
> - pg_get_transaction_extradata to pg_get_transaction_extra_data
> - pg_get_transaction_committime_data to pg_get_transaction_commit_time_data
> - pg_get_latest_transaction_committime_data to
> pg_get_latest_transaction_commit_time_data

Makes sense.

> 3) General remark: committs is not a name suited IMO (ts for
> transaction??). What if the code is changed to use commit_time instead?
> This remark counts as well for the file names committs.c and committs.h,
> and for pg_committs.

The ts is for timestamp, tx would be shorthand for transaction. Looking
at your remarks, it seems there is some general inconsistency with time
vs timestamp in this patch, we should pick one and stick with it.

> 6) To be consistent with everything, shouldn't track_commit_timestamp be
> renamed to track_commit_time

(see above)

> 9) Hm?! "oldest commit time xid", no?
> - "oldest running xid %u;
> %s",
> + "oldest CommitTs xid:
> %u; oldest running xid %u; %s",

Again, timestamp vs time.

> 12) In committs_desc(at)committsdesc(dot)c, isn't this block overthinking a bit:
> + else
> + appendStringInfo(buf, "UNKNOWN");
> It may be better to remove it, no?

Should be safe, indeed.

> 13) Isn't that 2014?
> + * Portions Copyright (c) 1996-2013, PostgreSQL Global Development Group

Hah, I forgot to update that (shows how long this patch has been waiting
:) )

> 16) make installcheck (test committs_off) fails on a server that has
> track_committs set to on. You should use an alternate output. I would

Well, it is supposed to fail, that's the whole point, the output should
be different depending on the value of the GUC.

> recommend removing as well the _off suffix in the test name. Let's use
> commit_time. Also, it should be mentioned in parallel_schedule with a
> comment that this test should always run alone and never in parallel
> with other tests. Honestly, I also think that test_committs brings no
> additional value and results in duplication code between
> src/test/regress and contrib/test_committs. So I'd just rip it off. On

Those tests are different though, one tests that the default (off) works
as expected the contrib one tests that the feature when turned on works
as expected. Since we can only set config values for contrib tests I
don't see how else to do this, but I am open to suggestions.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 01/11/14 12:19, Petr Jelinek wrote:
> Hi,
>
> thanks for review.
>
> On 01/11/14 05:45, Michael Paquier wrote:
>> Now here are a couple of comments at code level, this code seems not
>> enough baked for a commit:
>> 1) The following renaming should be done:
>> - pg_get_transaction_committime to pg_get_transaction_commit_time
>> - pg_get_transaction_extradata to pg_get_transaction_extra_data
>> - pg_get_transaction_committime_data to
>> pg_get_transaction_commit_time_data
>> - pg_get_latest_transaction_committime_data to
>> pg_get_latest_transaction_commit_time_data
>
> Makes sense.
>

On second thought, maybe those should be pg_get_transaction_committs,
pg_get_transaction_committs_data, etc.

For me the commit time thing feels problematic in the way I perceive it
- I see commit time as a point in time, where I see commit timestamp (or
committs for short) as something that can recorded. So I would prefer to
stick with commit timestamp/committs.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Nov 1, 2014 at 9:04 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:

> On 01/11/14 12:19, Petr Jelinek wrote:
>
>> Hi,
>>
>> thanks for review.
>>
>> On 01/11/14 05:45, Michael Paquier wrote:
>>
>>> Now here are a couple of comments at code level, this code seems not
>>> enough baked for a commit:
>>> 1) The following renaming should be done:
>>> - pg_get_transaction_committime to pg_get_transaction_commit_time
>>> - pg_get_transaction_extradata to pg_get_transaction_extra_data
>>> - pg_get_transaction_committime_data to
>>> pg_get_transaction_commit_time_data
>>> - pg_get_latest_transaction_committime_data to
>>> pg_get_latest_transaction_commit_time_data
>>>
>>
>> Makes sense.
>>
>>
> On second thought, maybe those should be pg_get_transaction_committs,
> pg_get_transaction_committs_data, etc.
> For me the commit time thing feels problematic in the way I perceive it -
> I see commit time as a point in time, where I see commit timestamp (or
> committs for short) as something that can recorded. So I would prefer to
> stick with commit timestamp/committs.

Hehe, I got exactly the opposite impression while reading the patch, but
let's rely on your judgement for the namings. I am not the one writing this
code.
--
Michael

On Sat, Nov 1, 2014 at 1:45 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> I am still planning to do more extensive tests, and study a bit more
> committs.c (with even more comments) as it is the core part of the feature.
>
More comments:
- Heikki already mentioned it, but after reading the code I see little
point in having the extra field implementing like that in core for many
reasons even if it is *just* 4 bytes:
1) It is untested and actually there is no direct use for it in core.
2) Pushing code that we know as dead is no good, that's a feature more or
less defined as maybe-useful-but-we-are-not-sure-yet-what-to-do-with-it.
3) If you're going to re-use this API in BDR, which is a fork of Postgres.
You'd better complete this API in BDR by yourself and not bother core with
that.
For those reasons I think that this extra field should be ripped off from
the patch.
- The API to get the commit timestamp is not that user-friendly, and I
think it could really be improved, to something like that for example:
pg_get_commit_timestamp(from_xact xid, number_of_xacts int);
pg_get_commit_timestamp(from_xact xid);
pg_get_commit_timestamp(); or pg_get_latest_commit_timestamp();
from_xact to NULL means latest. number_of_xacts to NULL means 1.
Comment in line with the fact that extra field is well, not really useful.
Regards,
--
Michael

On 01/11/14 14:00, Michael Paquier wrote:
>
> More comments:
> - Heikki already mentioned it, but after reading the code I see little
> point in having the extra field implementing like that in core for many
> reasons even if it is *just* 4 bytes:
> 1) It is untested and actually there is no direct use for it in core.
> 2) Pushing code that we know as dead is no good, that's a feature more
> or less defined as maybe-useful-but-we-are-not-sure-yet-what-to-do-with-it.
> 3) If you're going to re-use this API in BDR, which is a fork of
> Postgres. You'd better complete this API in BDR by yourself and not
> bother core with that.
> For those reasons I think that this extra field should be ripped off
> from the patch.

Well this is not BDR specific thing, the idea is that with logical
replication, commit timestamp is not enough for conflict handling, you
also need to have additional info in order to identify some types of
conflicts conflicts (local update vs remote update for example). So the
extradata field was meant as something that could be used to add the
additional info to the xid.

But I see your point, I think solving this issue can be left to the
replication identifier patch that is discussed in separate thread.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2014-11-01 13:45:44 +0900, Michael Paquier wrote:
> 14) I'd put the two checks in the reverse order:
> + Assert(xid != InvalidTransactionId);
> +
> + if (!commit_ts_enabled)
> + return;

Please don't. The order is correct right now. Why you ask? This way the
correctness of the callsites is checked even when committs is
disabled. Which it'll likely be on the majority of developer setups. And
what's the upsite of changing the order? There's no difference in the
generated code in production builds and the overhead in assert enabled
ones is neglegible.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2014-11-01 22:00:40 +0900, Michael Paquier wrote:
> On Sat, Nov 1, 2014 at 1:45 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
>
> > I am still planning to do more extensive tests, and study a bit more
> > committs.c (with even more comments) as it is the core part of the feature.
> >
> More comments:
> - Heikki already mentioned it, but after reading the code I see little
> point in having the extra field implementing like that in core for many
> reasons even if it is *just* 4 bytes:
> 1) It is untested and actually there is no direct use for it in core.

Meh. The whole feature is only there for extensions, not core.

> 2) Pushing code that we know as dead is no good, that's a feature more or
> less defined as maybe-useful-but-we-are-not-sure-yet-what-to-do-with-it.

Uh. It's not more/less dead than the whole of committs.

> 3) If you're going to re-use this API in BDR, which is a fork of Postgres.
> You'd better complete this API in BDR by yourself and not bother core with
> that.

I think that's a fundamentally wrong position. The only reason BDR isn't
purely stock postgres is that some features couldn't sanely be made work
without patches. I *hate* the fact that we had to do so. And I really
hope that we don't need any of the patches we have when building against
9.5.

So, now you might argue that the additional data is useless. But I think
that's just not thought far enough. If you think about it, in which
scenarios do you want to map xids to the commit timestamp? Primarily
that's going to be replication, right? One of the most obvious usecases
is allowing to detect/analyze/resolve conflicts in a multimaster setup,
right? To make sensible decisisons you'll often want to have more
information about the involved transactions. Makes sense so far?

Now, you might argue that could just be done with some table
transaction_metadata(xid DEFAULT txid_current(), meta, data). But that
has *significant* disadvantages: For one, it'll not work correctly once
subtransactions are used. Not good. For another it has about a
magnitude higher overhead than the committs way.

And it's not like the the extra field is in any way bdr specific - even
if you actually want to store much more information about the
transaction than just the origin (which is what bdr does), you can use
it to correctly solve the subtransaction problem and refer to some
transaction metadata table.

> - The API to get the commit timestamp is not that user-friendly, and I
> think it could really be improved, to something like that for example:
> pg_get_commit_timestamp(from_xact xid, number_of_xacts int);

What'd be the point of this?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2014-11-01 14:41:02 +0100, Petr Jelinek wrote:
> On 01/11/14 14:00, Michael Paquier wrote:
> >
> >More comments:
> >- Heikki already mentioned it, but after reading the code I see little
> >point in having the extra field implementing like that in core for many
> >reasons even if it is *just* 4 bytes:
> >1) It is untested and actually there is no direct use for it in core.
> >2) Pushing code that we know as dead is no good, that's a feature more
> >or less defined as maybe-useful-but-we-are-not-sure-yet-what-to-do-with-it.
> >3) If you're going to re-use this API in BDR, which is a fork of
> >Postgres. You'd better complete this API in BDR by yourself and not
> >bother core with that.
> >For those reasons I think that this extra field should be ripped off
> >from the patch.
>
> Well this is not BDR specific thing, the idea is that with logical
> replication, commit timestamp is not enough for conflict handling, you also
> need to have additional info in order to identify some types of conflicts
> conflicts (local update vs remote update for example). So the extradata
> field was meant as something that could be used to add the additional info
> to the xid.
>
> But I see your point, I think solving this issue can be left to the
> replication identifier patch that is discussed in separate thread.

For me this really hasn't anything directly to do with replication
identifiers, so delaying this decision doesn't make sense to me.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 01/11/14 18:44, Andres Freund wrote:
> On 2014-11-01 22:00:40 +0900, Michael Paquier wrote:
>> On Sat, Nov 1, 2014 at 1:45 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
>> wrote:
>>
>>> I am still planning to do more extensive tests, and study a bit more
>>> committs.c (with even more comments) as it is the core part of the feature.
>>>
>> More comments:
>> - Heikki already mentioned it, but after reading the code I see little
>> point in having the extra field implementing like that in core for many
>> reasons even if it is *just* 4 bytes:
>> 1) It is untested and actually there is no direct use for it in core.
>
> Meh. The whole feature is only there for extensions, not core.
>
>> 2) Pushing code that we know as dead is no good, that's a feature more or
>> less defined as maybe-useful-but-we-are-not-sure-yet-what-to-do-with-it.
>
> Uh. It's not more/less dead than the whole of committs.
>
>> 3) If you're going to re-use this API in BDR, which is a fork of Postgres.
>> You'd better complete this API in BDR by yourself and not bother core with
>> that.
>
> I think that's a fundamentally wrong position. The only reason BDR isn't
> purely stock postgres is that some features couldn't sanely be made work
> without patches. I *hate* the fact that we had to do so. And I really
> hope that we don't need any of the patches we have when building against
> 9.5.
>
> So, now you might argue that the additional data is useless. But I think
> that's just not thought far enough. If you think about it, in which
> scenarios do you want to map xids to the commit timestamp? Primarily
> that's going to be replication, right? One of the most obvious usecases
> is allowing to detect/analyze/resolve conflicts in a multimaster setup,
> right? To make sensible decisisons you'll often want to have more
> information about the involved transactions. Makes sense so far?
>
> Now, you might argue that could just be done with some table
> transaction_metadata(xid DEFAULT txid_current(), meta, data). But that
> has *significant* disadvantages: For one, it'll not work correctly once
> subtransactions are used. Not good. For another it has about a
> magnitude higher overhead than the committs way.
>
> And it's not like the the extra field is in any way bdr specific - even
> if you actually want to store much more information about the
> transaction than just the origin (which is what bdr does), you can use
> it to correctly solve the subtransaction problem and refer to some
> transaction metadata table.
>

Well, Michael has point that the extradata is pretty much useless
currently, perhaps it would help to add the interface to set extradata?

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 11/1/14, 8:41 AM, Petr Jelinek wrote:
> Well this is not BDR specific thing, the idea is that with logical replication, commit timestamp is not enough for conflict handling, you also need to have additional info in order to identify some types of conflicts conflicts (local update vs remote update for example). So the extradata field was meant as something that could be used to add the additional info to the xid.

Related to this... is there any way to deal with 2 transactions that commit in the same microsecond? It seems silly to try and handle that for every commit since it should be quite rare, but perhaps we could store the LSN as extradata if we detect a conflict?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

Jim Nasby wrote:
> On 11/1/14, 8:41 AM, Petr Jelinek wrote:
> >Well this is not BDR specific thing, the idea is that with logical replication, commit timestamp is not enough for conflict handling, you also need to have additional info in order to identify some types of conflicts conflicts (local update vs remote update for example). So the extradata field was meant as something that could be used to add the additional info to the xid.
>
> Related to this... is there any way to deal with 2 transactions that commit in the same microsecond? It seems silly to try and handle that for every commit since it should be quite rare, but perhaps we could store the LSN as extradata if we detect a conflict?

Well, two things. One, LSN is 8 bytes and extradata (at least in this
patch when I last saw it) is only 4 bytes. But secondly and more
important is that detecting a conflict is done in node B *after* node A
has recorded the transaction's commit time; there is no way to know at
commit time that there is going to be a conflict caused by that
transaction in the future. (If there was a way to tell, you could just
as well not commit the transaction in the first place.)

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

On 11/1/14 8:04 AM, Petr Jelinek wrote:
> On second thought, maybe those should be pg_get_transaction_committs,
> pg_get_transaction_committs_data, etc.

Please don't name anything "committs". That looks like a misspelling of
something.

There is nothing wrong with

pg_get_transaction_commit_timestamp()

If you want to reduce the length, lose the "get".

> For me the commit time thing feels problematic in the way I perceive it
> - I see commit time as a point in time, where I see commit timestamp (or
> committs for short) as something that can recorded. So I would prefer to
> stick with commit timestamp/committs.

In PostgreSQL, it is pretty clearly established that time is hours,
minutes, seconds, and timestamp is years, months, days, hours, minutes,
seconds. So unless this feature only records the hour, minute, and
second of a commit, it should be "timestamp".

On 03/11/14 22:26, Peter Eisentraut wrote:
> On 11/1/14 8:04 AM, Petr Jelinek wrote:
>> On second thought, maybe those should be pg_get_transaction_committs,
>> pg_get_transaction_committs_data, etc.
>
> Please don't name anything "committs". That looks like a misspelling of
> something.
>
> There is nothing wrong with
>
> pg_get_transaction_commit_timestamp()
>
> If you want to reduce the length, lose the "get".
>

I am fine with that, I only wonder if your definition of "anything" only
concerns the SQL interfaces or also the internals.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Nov 3, 2014 at 3:26 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 11/1/14 8:04 AM, Petr Jelinek wrote:
>> On second thought, maybe those should be pg_get_transaction_committs,
>> pg_get_transaction_committs_data, etc.
>
> Please don't name anything "committs". That looks like a misspelling of
> something.
>
> There is nothing wrong with
>
> pg_get_transaction_commit_timestamp()
>
> If you want to reduce the length, lose the "get".

+1: all non void returning functions 'get' something.

>> For me the commit time thing feels problematic in the way I perceive it
>> - I see commit time as a point in time, where I see commit timestamp (or
>> committs for short) as something that can recorded. So I would prefer to
>> stick with commit timestamp/committs.
>
> In PostgreSQL, it is pretty clearly established that time is hours,
> minutes, seconds, and timestamp is years, months, days, hours, minutes,
> seconds. So unless this feature only records the hour, minute, and
> second of a commit, it should be "timestamp".

Elsewhere, for example, we have: "pg_last_xact_replay_timestamp()".
So, in keeping with that, maybe,

pg_xact_commit_timestamp(xid)

merlin

On 2014-11-02 19:27:25 +0100, Petr Jelinek wrote:
> Well, Michael has point that the extradata is pretty much useless currently,
> perhaps it would help to add the interface to set extradata?

Only accessible via C and useless aren't the same thing. But sure, add
it.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Nov 1, 2014 at 10:00 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> More comments:
>

I have done a couple of tests on my laptop with pgbench like that to
generate a maximum of transaction commits:
$ pgbench --no-vacuum -f ~/Desktop/commit.sql -T 60 -c 24 -j 24
$ cat ~/Desktop/commit.sql
SELECT txid_current()
Here is an average of 5 runs:
- master: 49842.44
- GUC off, patched = 49688.71
- GUC on, patched = 49459.73
So there is little noise.

Here are also more comments about the code that I found while focusing on
committs.c:
1) When the GUC is not enabled, and when the transaction ID provided is not
in a correct range, a dummy value based on InvalidTransactionId (?!) is
returned, like that:
+ /* Return empty if module not enabled */
+ if (!commit_ts_enabled)
+ {
+ if (ts)
+ *ts = InvalidTransactionId;
+ if (data)
+ *data = (CommitExtraData) 0;
+ return;
+ }
This leads to some incorrect results:
=# select pg_get_transaction_committime('1');
pg_get_transaction_committime
-------------------------------
2000-01-01 09:00:00+09
(1 row)
Or for example:
=# SELECT txid_current();
txid_current
--------------
1006
(1 row)
=# select pg_get_transaction_committime('1006');
pg_get_transaction_committime
-------------------------------
2014-11-04 14:54:37.589381+09
(1 row)
=# select pg_get_transaction_committime('1007');
pg_get_transaction_committime
-------------------------------
2000-01-01 09:00:00+09
(1 row)
=# SELECT txid_current();
txid_current
--------------
1007
(1 row)
And at other times error is not that helpful for user:
=# select pg_get_transaction_committime('10000');
ERROR: XX000: could not access status of transaction 10000
DETAIL: Could not read from file "pg_committs/0000" at offset 114688:
Undefined error: 0.
LOCATION: SlruReportIOError, slru.c:896
I think that you should simply return an error in
TransactionIdGetCommitTsData when xid is not in the range supported. That
will be more transparent for the user.
2) You may as well want to return a different error if the GUC
track_commit_timestamps is disabled.
3) This comment should be updated in committs.c, we are not talking about
CLOG here:
+/*
+ * Link to shared-memory data structures for CLOG control
+ */
4) Similarly, I think more comments should be put in here. It is OK to
truncate the commit timestamp stuff similarly to CLOG to have a consistent
status context available, but let's explain it.
* multixacts; that will be done by the next checkpoint.
*/
TruncateCLOG(frozenXID);
+ TruncateCommitTs(frozenXID)
5) Reading the code, TransactionTreeSetCommitTimestamp is always called
with do_xlog = false, making that actually no timestamps are WAL'd... Hence
WriteSetTimestampXlogRec is just dead code with the latest version of the
patch. IMO, this should be always WAL-logged when track_commit_timestamp is
on.
6) Shouldn't any value update of track_commit_timestamp be tracked in
XLogReportParameters? That's thinking about making the commit timestamp
available on standbys as well..
7) pg_xlogdump has no support for RM_COMMITTS_ID, something that would be
useful for developers.
8) The redo and xlog routines of committs should be out in a dedicated
file, like committsxlog.c or similar. The other rmgr do so, so let's be
consistent.

Regards,
--
Michael

On 2014-11-04 17:19:18 +0900, Michael Paquier wrote:
> 5) Reading the code, TransactionTreeSetCommitTimestamp is always called
> with do_xlog = false, making that actually no timestamps are WAL'd... Hence
> WriteSetTimestampXlogRec is just dead code with the latest version of the
> patch. IMO, this should be always WAL-logged when track_commit_timestamp is
> on.

It's callable via a 'extern' function. So, I'd not consider it dead. And
the WAL logging is provided by xact.c's own WAL logging - it always does
the corresponding committs calls.

> 6) Shouldn't any value update of track_commit_timestamp be tracked in
> XLogReportParameters? That's thinking about making the commit timestamp
> available on standbys as well..

Yes, it should.

> 7) pg_xlogdump has no support for RM_COMMITTS_ID, something that would be
> useful for developers.

What do you mean by that? There's the corresponding rmgrdesc.c support I
think?

> 8) The redo and xlog routines of committs should be out in a dedicated
> file, like committsxlog.c or similar. The other rmgr do so, so let's be
> consistent.

Seems pointless to me. The file isn't that big and the other SLRUs don't
do it.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Nov 4, 2014 at 5:05 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:

> On 2014-11-02 19:27:25 +0100, Petr Jelinek wrote:
> > Well, Michael has point that the extradata is pretty much useless
> currently,
> > perhaps it would help to add the interface to set extradata?
>
> Only accessible via C and useless aren't the same thing. But sure, add
> it.
>
I'm still on a -1 for that. You mentioned that there is perhaps no reason
to delay a decision on this matter, but IMO there is no reason to rush
either in doing something we may regret. And I am not the only one on this
thread expressing concern about this extra data thingy.

If this extra data field is going to be used to identify from which node a
commit comes from, then it is another feature than what is written on the
subject of this thread. In this case let's discuss it in the thread
dedicated to replication identifiers, or come up with an extra patch once
the feature for commit timestamps is done.
--
Michael

On Tue, Nov 4, 2014 at 5:23 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:

> On 2014-11-04 17:19:18 +0900, Michael Paquier wrote:
> > 5) Reading the code, TransactionTreeSetCommitTimestamp is always called
> > with do_xlog = false, making that actually no timestamps are WAL'd...
> Hence
> > WriteSetTimestampXlogRec is just dead code with the latest version of the
> > patch. IMO, this should be always WAL-logged when track_commit_timestamp
> is
> > on.
>
> It's callable via a 'extern' function. So, I'd not consider it dead. And
> the WAL logging is provided by xact.c's own WAL logging - it always does
> the corresponding committs calls.
>
The code path is unused. We'd better make the XLOG record mandatory if
tracking is enabled, as this information is useful on standbys as well.

> > 7) pg_xlogdump has no support for RM_COMMITTS_ID, something that would be
> > useful for developers.
>
> What do you mean by that? There's the corresponding rmgrdesc.c support I
> think?
>
Oops sorry. I thought there was some big switch in pg_xlogdump when writing
this comment. Yeah that's fine.
--
Michael

On 2014-11-04 17:29:04 +0900, Michael Paquier wrote:
> On Tue, Nov 4, 2014 at 5:23 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
> wrote:
>
> > On 2014-11-04 17:19:18 +0900, Michael Paquier wrote:
> > > 5) Reading the code, TransactionTreeSetCommitTimestamp is always called
> > > with do_xlog = false, making that actually no timestamps are WAL'd...
> > Hence
> > > WriteSetTimestampXlogRec is just dead code with the latest version of the
> > > patch. IMO, this should be always WAL-logged when track_commit_timestamp
> > is
> > > on.
> >
> > It's callable via a 'extern' function. So, I'd not consider it dead. And
> > the WAL logging is provided by xact.c's own WAL logging - it always does
> > the corresponding committs calls.
> >
> The code path is unused.

No. It is not. It can be called by extensions?

> We'd better make the XLOG record mandatory if
> tracking is enabled, as this information is useful on standbys as well.

Did you read what I wrote? To quote "And the WAL logging is provided by
xact.c's own WAL logging - it always does the corresponding committs
calls.".

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Michael Paquier wrote:

> I'm still on a -1 for that. You mentioned that there is perhaps no reason
> to delay a decision on this matter, but IMO there is no reason to rush
> either in doing something we may regret. And I am not the only one on this
> thread expressing concern about this extra data thingy.
>
> If this extra data field is going to be used to identify from which node a
> commit comes from, then it is another feature than what is written on the
> subject of this thread. In this case let's discuss it in the thread
> dedicated to replication identifiers, or come up with an extra patch once
> the feature for commit timestamps is done.

Introducing the extra data field in a later patch would mean an on-disk
representation change, i.e. pg_upgrade trouble.

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

On 2014-11-04 10:01:00 -0300, Alvaro Herrera wrote:
> Michael Paquier wrote:
>
> > I'm still on a -1 for that. You mentioned that there is perhaps no reason
> > to delay a decision on this matter, but IMO there is no reason to rush
> > either in doing something we may regret. And I am not the only one on this
> > thread expressing concern about this extra data thingy.
> >
> > If this extra data field is going to be used to identify from which node a
> > commit comes from, then it is another feature than what is written on the
> > subject of this thread. In this case let's discuss it in the thread
> > dedicated to replication identifiers, or come up with an extra patch once
> > the feature for commit timestamps is done.
>
> Introducing the extra data field in a later patch would mean an on-disk
> representation change, i.e. pg_upgrade trouble.

It's also simply not necessarily related to replication
identifiers. This is useful whether replication identifiers make it in
or not. It allows to implement something like replication identifiers
outside of core (albeit with a hefty overhead in OLTP workloads).

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 04/11/14 09:05, Andres Freund wrote:
> On 2014-11-02 19:27:25 +0100, Petr Jelinek wrote:
>> Well, Michael has point that the extradata is pretty much useless currently,
>> perhaps it would help to add the interface to set extradata?
>
> Only accessible via C and useless aren't the same thing. But sure, add
> it.
>

I actually meant nicer C api - the one that will make it possible to say
for this transaction, use this extradata (or for all transactions from
now on done by this backend use this extradata), instead of current API
where you have to overwrite what RecordCommit already wrote.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 04/11/14 09:19, Michael Paquier wrote:
> On Sat, Nov 1, 2014 at 10:00 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com <mailto:michael(dot)paquier(at)gmail(dot)com>> wrote:
>
> More comments:
>
> Here are also more comments about the code that I found while focusing
> on committs.c:
> 1) When the GUC is not enabled, and when the transaction ID provided is
> not in a correct range, a dummy value based on InvalidTransactionId (?!)
> is returned, like that:
> + /* Return empty if module not enabled */
> + if (!commit_ts_enabled)
> + {
> + if (ts)
> + *ts = InvalidTransactionId;
> + if (data)
> + *data = (CommitExtraData) 0;
> + return;
> + }
> This leads to some incorrect results:
> =# select pg_get_transaction_committime('1');
> pg_get_transaction_committime
> -------------------------------
> 2000-01-01 09:00:00+09
> (1 row)

Oh, I had this on my TODO and somehow forgot about it, I am leaning
towards throwing an error when calling any committs "get" function while
the tracking is disabled.

> Or for example:
> =# SELECT txid_current();
> txid_current
> --------------
> 1006
> (1 row)
> =# select pg_get_transaction_committime('1006');
> pg_get_transaction_committime
> -------------------------------
> 2014-11-04 14:54:37.589381+09
> (1 row)
> =# select pg_get_transaction_committime('1007');
> pg_get_transaction_committime
> -------------------------------
> 2000-01-01 09:00:00+09
> (1 row)
> =# SELECT txid_current();
> txid_current
> --------------
> 1007
> (1 row)
> And at other times error is not that helpful for user:
> =# select pg_get_transaction_committime('10000');
> ERROR: XX000: could not access status of transaction 10000
> DETAIL: Could not read from file "pg_committs/0000" at offset 114688:
> Undefined error: 0.
> LOCATION: SlruReportIOError, slru.c:896
> I think that you should simply return an error in
> TransactionIdGetCommitTsData when xid is not in the range supported.
> That will be more transparent for the user.

Agreed.

> 5) Reading the code, TransactionTreeSetCommitTimestamp is always called
> with do_xlog = false, making that actually no timestamps are WAL'd...
> Hence WriteSetTimestampXlogRec is just dead code with the latest version
> of the patch. IMO, this should be always WAL-logged when
> track_commit_timestamp is on.

As Andres explained this is always WAL-logged when called by current
caller so we don't want it to be double logged, so that's why do_xlog =
false, but when extension will need call it, it will most likely need
do_xlog = true.

> 8) The redo and xlog routines of committs should be out in a dedicated
> file, like committsxlog.c or similar. The other rmgr do so, so let's be
> consistent.
>

Most actually don't AFAICS.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 11/3/14 5:17 PM, Petr Jelinek wrote:
>> Please don't name anything "committs". That looks like a misspelling of
>> something.
>>
>> There is nothing wrong with
>>
>> pg_get_transaction_commit_timestamp()
>>
>> If you want to reduce the length, lose the "get".
>>
>
> I am fine with that, I only wonder if your definition of "anything" only
> concerns the SQL interfaces or also the internals.

I'd be fine with commit_ts for internals, but not committs.

One day, you'll need a function or data structure that works with
multiple of these, and then you'll really be in naming trouble. ;-)

On 04/11/14 09:25, Michael Paquier wrote:
> On Tue, Nov 4, 2014 at 5:05 PM, Andres Freund <andres(at)2ndquadrant(dot)com
> <mailto:andres(at)2ndquadrant(dot)com>> wrote:
>
> On 2014-11-02 19:27:25 +0100, Petr Jelinek wrote:
> > Well, Michael has point that the extradata is pretty much useless currently,
> > perhaps it would help to add the interface to set extradata?
>
> Only accessible via C and useless aren't the same thing. But sure, add
> it.
>
> I'm still on a -1 for that. You mentioned that there is perhaps no
> reason to delay a decision on this matter, but IMO there is no reason to
> rush either in doing something we may regret. And I am not the only one
> on this thread expressing concern about this extra data thingy.
>
> If this extra data field is going to be used to identify from which node
> a commit comes from, then it is another feature than what is written on
> the subject of this thread. In this case let's discuss it in the thread
> dedicated to replication identifiers, or come up with an extra patch
> once the feature for commit timestamps is done.

The issue as I see it is that both of those features are closely related
and just one without the other has very limited use. What I learned from
working on UDR is that for conflict handling, I was actually missing the
extradata more than the timestamp itself - in other words I have
extension for 9.4 where I have use for this API already, so the argument
about dead code or forks or whatever does not really hold.

The other problem is that if we add extradata later we will either break
upgrade-ability of will have to write essentially same code again which
will store just the extradata instead of the timestamp, I don't really
like either of those options to be honest.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 04/11/14 22:20, Peter Eisentraut wrote:
> On 11/3/14 5:17 PM, Petr Jelinek wrote:
>>> Please don't name anything "committs". That looks like a misspelling of
>>> something.
>>>
>>> There is nothing wrong with
>>>
>>> pg_get_transaction_commit_timestamp()
>>>
>>> If you want to reduce the length, lose the "get".
>>>
>>
>> I am fine with that, I only wonder if your definition of "anything" only
>> concerns the SQL interfaces or also the internals.
>
> I'd be fine with commit_ts for internals, but not committs.
>
> One day, you'll need a function or data structure that works with
> multiple of these, and then you'll really be in naming trouble. ;-)
>

Hmm we use CommitTs in interfaces that uses CamelCase naming so I guess
commit_ts is indeed natural expansion of that.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Nov 4, 2014 at 10:01 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Michael Paquier wrote:
>
> > I'm still on a -1 for that. You mentioned that there is perhaps no reason
> > to delay a decision on this matter, but IMO there is no reason to rush
> > either in doing something we may regret. And I am not the only one on
> this
> > thread expressing concern about this extra data thingy.
> >
> > If this extra data field is going to be used to identify from which node
> a
> > commit comes from, then it is another feature than what is written on the
> > subject of this thread. In this case let's discuss it in the thread
> > dedicated to replication identifiers, or come up with an extra patch once
> > the feature for commit timestamps is done.
>
> Introducing the extra data field in a later patch would mean an on-disk
> representation change, i.e. pg_upgrade trouble.

Then why especially 4 bytes for the extra field? Why not 8 or 16?
--
Michael

On 2014-11-05 08:57:07 +0900, Michael Paquier wrote:
> On Tue, Nov 4, 2014 at 10:01 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
> wrote:
>
> > Michael Paquier wrote:
> >
> > > I'm still on a -1 for that. You mentioned that there is perhaps no reason
> > > to delay a decision on this matter, but IMO there is no reason to rush
> > > either in doing something we may regret. And I am not the only one on
> > this
> > > thread expressing concern about this extra data thingy.
> > >
> > > If this extra data field is going to be used to identify from which node
> > a
> > > commit comes from, then it is another feature than what is written on the
> > > subject of this thread. In this case let's discuss it in the thread
> > > dedicated to replication identifiers, or come up with an extra patch once
> > > the feature for commit timestamps is done.
> >
> > Introducing the extra data field in a later patch would mean an on-disk
> > representation change, i.e. pg_upgrade trouble.
>
> Then why especially 4 bytes for the extra field? Why not 8 or 16?

It's sufficiently long that you can build infrastructure to storing more
transaction metadata data ontop. I could live making it 8 bytes, but I
don't see a clear advantage.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 11/3/14, 2:36 PM, Alvaro Herrera wrote:
> Jim Nasby wrote:
>> On 11/1/14, 8:41 AM, Petr Jelinek wrote:
>>> Well this is not BDR specific thing, the idea is that with logical replication, commit timestamp is not enough for conflict handling, you also need to have additional info in order to identify some types of conflicts conflicts (local update vs remote update for example). So the extradata field was meant as something that could be used to add the additional info to the xid.
>>
>> Related to this... is there any way to deal with 2 transactions that commit in the same microsecond? It seems silly to try and handle that for every commit since it should be quite rare, but perhaps we could store the LSN as extradata if we detect a conflict?
>
> Well, two things. One, LSN is 8 bytes and extradata (at least in this
> patch when I last saw it) is only 4 bytes. But secondly and more
> important is that detecting a conflict is done in node B *after* node A
> has recorded the transaction's commit time; there is no way to know at
> commit time that there is going to be a conflict caused by that
> transaction in the future. (If there was a way to tell, you could just
> as well not commit the transaction in the first place.)

I'm worried about 2 commits in the same microsecond on the same system, not on 2 different systems. Or, put another way, if we're going to expose this I think it should also provide a guaranteed unique commit ordering for a single cluster. Presumably, this shouldn't be that hard since we do know the exact order in which things committed.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Tue, 2014-11-04 at 23:43 -0600, Jim Nasby wrote:

> I'm worried about 2 commits in the same microsecond on the same
> system, not on 2 different systems. Or, put another way, if we're
> going to expose this I think it should also provide a guaranteed
> unique commit ordering for a single cluster. Presumably, this
> shouldn't be that hard since we do know the exact order in which
> things committed.

Addition of LSN when the timestamps for two transactions are exactly
same isn't enough. There isn't any guarantee that a later commit gets a
later timestamp than an earlier commit.

In addition, I wonder if this feature would be misused. Record
transaction ids to a table to find out commit order (use case could be
storing historical row versions for example). Do a dump and restore on
another cluster, and all the transaction ids are completely meaningless
to the system.

Having the ability to record commit order into an audit table would be
extremely welcome, but as is, this feature doesn't provide it.

- Anssi

On Wed, Nov 5, 2014 at 5:24 PM, Anssi Kääriäinen <anssi(dot)kaariainen(at)thl(dot)fi>
wrote:

> On Tue, 2014-11-04 at 23:43 -0600, Jim Nasby wrote:
>
> > I'm worried about 2 commits in the same microsecond on the same
> > system, not on 2 different systems. Or, put another way, if we're
> > going to expose this I think it should also provide a guaranteed
> > unique commit ordering for a single cluster. Presumably, this
> > shouldn't be that hard since we do know the exact order in which
> > things committed.
>
> Addition of LSN when the timestamps for two transactions are exactly
> same isn't enough. There isn't any guarantee that a later commit gets a
> later timestamp than an earlier commit.
>
True if WAL record ID is not globally consistent. Two-level commit ordering
can be done with (timestamp or LSN, nodeID). At equal timestamp, we could
say as well that the node with the lowest systemID wins for example. That's
not something

> In addition, I wonder if this feature would be misused. Record
> transaction ids to a table to find out commit order (use case could be
> storing historical row versions for example). Do a dump and restore on
> another cluster, and all the transaction ids are completely meaningless
> to the system.
>
I think you are forgetting the fact to be able to take a consistent dump
using an exported snapshot. In this case the commit order may not be that
meaningless..

> Having the ability to record commit order into an audit table would be
> extremely welcome, but as is, this feature doesn't provide it.
>
That's something that can actually be achieved with this feature if the SQL
interface is able to query all the timestamps in a xid range with for
example a background worker that tracks this data periodically. Now the
thing is as well: how much timestamp history do we want to keep? The patch
truncating SLRU files with frozenID may cover a sufficient range...
--
Michael

On 11/5/14, 6:10 AM, Michael Paquier wrote:
> In addition, I wonder if this feature would be misused. Record
> transaction ids to a table to find out commit order (use case could be
> storing historical row versions for example). Do a dump and restore on
> another cluster, and all the transaction ids are completely meaningless
> to the system.
>
> I think you are forgetting the fact to be able to take a consistent dump using an exported snapshot. In this case the commit order may not be that meaningless..

Anssi's point is that you can't use xmin because it can change, but I think anyone working with this feature would understand that.

> Having the ability to record commit order into an audit table would be
> extremely welcome, but as is, this feature doesn't provide it.
>
> That's something that can actually be achieved with this feature if the SQL interface is able to query all the timestamps in a xid range with for example a background worker that tracks this data periodically. Now the thing is as well: how much timestamp history do we want to keep? The patch truncating SLRU files with frozenID may cover a sufficient range...

Except that commit time is not guaranteed unique *even on a single system*. That's my whole point. If we're going to bother with all the commit time machinery it seems really silly to provide a way to uniquely order every commit.

Clearly trying to uniquely order commits across multiple systems is a far larger problem, and I'm not suggesting we attempt that. But for a single system AIUI all we need to do is expose the LSN of each commit record and that will give you the exact and unique order in which transactions committed.

This isn't a hypothetical feature either; if we had this, logical replication systems wouldn't have to try and fake this via batches. You could actually recreate exactly what data was visible at what time to all transactions, not just repeatable read ones (as long as you kept snapshot data as well, which isn't hard).

As for how much data to keep, if you have a process that's doing something to record this information permanently all it needs to do is keep an old enough snapshot around. That's not that hard to do, even from user space.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 2014-11-05 10:23:15 -0600, Jim Nasby wrote:
>
>
> On 11/5/14, 6:10 AM, Michael Paquier wrote:
> > In addition, I wonder if this feature would be misused. Record
> > transaction ids to a table to find out commit order (use case could be
> > storing historical row versions for example). Do a dump and restore on
> > another cluster, and all the transaction ids are completely meaningless
> > to the system.
> >
> >I think you are forgetting the fact to be able to take a consistent dump using an exported snapshot. In this case the commit order may not be that meaningless..
>
> Anssi's point is that you can't use xmin because it can change, but I think anyone working with this feature would understand that.
>
> > Having the ability to record commit order into an audit table would be
> > extremely welcome, but as is, this feature doesn't provide it.
> >
> >That's something that can actually be achieved with this feature if
> >the SQL interface is able to query all the timestamps in a xid range
> >with for example a background worker that tracks this data
> >periodically. Now the thing is as well: how much timestamp history do
> >we want to keep? The patch truncating SLRU files with frozenID may
> >cover a sufficient range...

> Except that commit time is not guaranteed unique *even on a single
> system*. That's my whole point. If we're going to bother with all the
> commit time machinery it seems really silly to provide a way to
> uniquely order every commit.

Well. I think that's the misunderstanding here. That's absolutely not
what committs is supposed to be used for. For the replication stream
you'd hopefully use logical decoding. That gives you the transaction
data exactly in commit order.

> Clearly trying to uniquely order commits across multiple systems is a
> far larger problem, and I'm not suggesting we attempt that. But for a
> single system AIUI all we need to do is expose the LSN of each commit
> record and that will give you the exact and unique order in which
> transactions committed.

I don't think that's something you should attempt. That's what logical
decoding is for. Hence I see little point in exposing the LSN that way.

Where I think committs is useful is a method for analyzing and resolving
conflicts between multiple systems. In that case you likely can't use
the LSN for anything as it won't be very meaningful. If you get
conflicts below the accuracy of the timestamps you better use another
deterministic method of resolving them - BDR e.g. compares the system
identifier, timeline id, database oid, and a user defined name. While
enforcing that those aren't the same between systems.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 11/5/14, 10:30 AM, Andres Freund wrote:
>> Except that commit time is not guaranteed unique *even on a single
>> >system*. That's my whole point. If we're going to bother with all the
>> >commit time machinery it seems really silly to provide a way to
>> >uniquely order every commit.
> Well. I think that's the misunderstanding here. That's absolutely not
> what committs is supposed to be used for. For the replication stream
> you'd hopefully use logical decoding. That gives you the transaction
> data exactly in commit order.

So presumably you'd want to use logical decoding to insert into a table with a sequence on it, or similar?

I agree, that sounds like a better way to handle this. I think it's worth mentioning in the docs for commit_ts, because people WILL mistakenly try and use it to determine commit ordering.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 2014-11-05 10:34:40 -0600, Jim Nasby wrote:
> On 11/5/14, 10:30 AM, Andres Freund wrote:
> >>Except that commit time is not guaranteed unique *even on a single
> >>>system*. That's my whole point. If we're going to bother with all the
> >>>commit time machinery it seems really silly to provide a way to
> >>>uniquely order every commit.

> >Well. I think that's the misunderstanding here. That's absolutely not
> >what committs is supposed to be used for. For the replication stream
> >you'd hopefully use logical decoding. That gives you the transaction
> >data exactly in commit order.
>
> So presumably you'd want to use logical decoding to insert into a
> table with a sequence on it, or similar?

I'm not following. I'd use logical decoding to replicate the data to
another system, thereby guaranteeing its done in commit order. Then,
when applying the data on the other side, I can detect/resolve some
forms of conflicts by looking at the timestamps of rows via committs.

> I agree, that sounds like a better way to handle this. I think it's
> worth mentioning in the docs for commit_ts, because people WILL
> mistakenly try and use it to determine commit ordering.

Ok, sounds sensible.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Jim Nasby <Jim(dot)Nasby(at)BlueTreble(dot)com> wrote:

> for a single system AIUI all we need to do is expose the LSN of
> each commit record and that will give you the exact and unique
> order in which transactions committed.
>
> This isn't a hypothetical feature either; if we had this,
> logical replication systems wouldn't have to try and fake this
> via batches. You could actually recreate exactly what data was
> visible at what time to all transactions, not just repeatable
> read ones (as long as you kept snapshot data as well, which isn't
> hard).

Well, that not entirely true for serializable transactions; there
are points in time where reading the committed state could cause a
transaction to roll back[1] -- either a writing transaction which
would make that visible state inconsistent with the later committed
state or the reading transaction if it views something which is not
(yet) consistent.

That's not to say that this feature is a bad idea; part of the
serializable implementation itself depends on being able to
accurately determine commit order, and this feature could allow
that to work more efficiently. I'm saying that, like hot standby,
a replicated database could not provide truly serializable
transactions (even read only ones) without something else in
addition to this. We've discussed various ways of doing that.
Perhaps the most promising is to include in the stream some
indication of which points in the transaction stream are safe for a
serializable transaction to read. If there's a way to implement
commit order recording such that a two-state flag could be
associated with each commit, I think it could be made to work for
serializable transactions.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

[1] https://wiki.postgresql.org/wiki/SSI#Read_Only_Transactions

On 11/05/2014 11:23 AM, Jim Nasby wrote:
>
>
> Except that commit time is not guaranteed unique *even on a single
> system*. That's my whole point. If we're going to bother with all the
> commit time machinery it seems really silly to provide a way to
> uniquely order every commit.
>
> Clearly trying to uniquely order commits across multiple systems is a
> far larger problem, and I'm not suggesting we attempt that. But for a
> single system AIUI all we need to do is expose the LSN of each commit
> record and that will give you the exact and unique order in which
> transactions committed.
>
> This isn't a hypothetical feature either; if we had this, logical
> replication systems wouldn't have to try and fake this via batches.
> You could actually recreate exactly what data was visible at what time
> to all transactions, not just repeatable read ones (as long as you
> kept snapshot data as well, which isn't hard).
>
> As for how much data to keep, if you have a process that's doing
> something to record this information permanently all it needs to do is
> keep an old enough snapshot around. That's not that hard to do, even
> from user space.

+1 for this.

It isn't just 'replication' systems that have a need for getting the
commit order of transactions on a single system. I have a application
(not slony) where we want to query a table but order the output based on
the transaction commit order of when the insert into the table was done
(think of a queue). I'm not replicating the output but passing the data
to other applications for further processing. If I just had the commit
timestamp I would need to put in some other condition to break ties in a
consistent way. I think being able to get an ordering by commit LSN is
what I really want in this case not the timestamp.

Logical decoding is one solution to this (that I was considering) but
being able to do something like
select * FROM event_log order by commit_id would be a lot simpler.

On 2014-11-05 17:17:05 -0500, Steve Singer wrote:
> It isn't just 'replication' systems that have a need for getting the commit
> order of transactions on a single system. I have a application (not slony)
> where we want to query a table but order the output based on the transaction
> commit order of when the insert into the table was done (think of a queue).
> I'm not replicating the output but passing the data to other applications
> for further processing. If I just had the commit timestamp I would need to
> put in some other condition to break ties in a consistent way. I think
> being able to get an ordering by commit LSN is what I really want in this
> case not the timestamp.
>
> Logical decoding is one solution to this (that I was considering) but being
> able to do something like
> select * FROM event_log order by commit_id would be a lot simpler.

Imo that's essentially a different feature. What you essentially would
need here is a 'commit sequence number' - but no timestamps. And
probably to be useful that number has to be 8 bytes in itself.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 11/05/2014 05:43 PM, Andres Freund wrote:
> On 2014-11-05 17:17:05 -0500, Steve Singer wrote:
> Imo that's essentially a different feature. What you essentially would
> need here is a 'commit sequence number' - but no timestamps. And
> probably to be useful that number has to be 8 bytes in itself.

I think this gets to the heart of some of the differing views people
have expressed on this patch

Is this patch supposed to:

A) Add commit timestamp tracking but nothing more

B) Add infrastructure to store commit timestamps and provide a facility
for storing additional bits of data extensions might want to be
associated with the commit

C). Add commit timestamps and node identifiers to commits

If the answer is (A) then I can see why some people are objecting to
adding extradata. If the answer is (B) then it's fair to ask how well
does this patch handle various types of things people might want to
attach to the commit record (such as the LSN). I think the problem is
that once you start providing a facility extensions can use to store
data along with the commit record then being restricted to 4 or 8 bytes
is very limiting. It also doesn't allow you to load two extensions at
once on a system. You wouldn't be able to have both the
'steve_commit_order' extension and BDR installed at the same time. I
don't think this patch does a very good job at (B) but It wasn't
intended to.

If what we are really doing is C, and just calling the space 'extradata'
until we get the logical identifier stuff in and then we are going to
rename extradata to nodeid then we should say so. If we are really
concerned about the pg_upgrade impact of expanding the record later then
maybe we should add 4 bytes of padding to the CommitTimeStampEntry now
and but leave the manipulating the node id until later.

Steve

> Greetings,
>
> Andres Freund
>

On 2014-11-05 19:31:52 -0500, Steve Singer wrote:
> On 11/05/2014 05:43 PM, Andres Freund wrote:
> >On 2014-11-05 17:17:05 -0500, Steve Singer wrote:
> >Imo that's essentially a different feature. What you essentially would
> >need here is a 'commit sequence number' - but no timestamps. And
> >probably to be useful that number has to be 8 bytes in itself.
>
> I think this gets to the heart of some of the differing views people have
> expressed on this patch

I think it's actually besides the heart...

> Is this patch supposed to:
>
> A) Add commit timestamp tracking but nothing more
>
> B) Add infrastructure to store commit timestamps and provide a facility for
> storing additional bits of data extensions might want to be associated with
> the commit
>
> C). Add commit timestamps and node identifiers to commits

> If the answer is (A) then I can see why some people are objecting to adding
> extradata. If the answer is (B) then it's fair to ask how well does this
> patch handle various types of things people might want to attach to the
> commit record (such as the LSN).

I think there's a mistake exactly here. The LSN of the commit isn't just
some extra information about the commit. You can't say 'here, also
attach this piece of information'. Instead you need special case code in
xact.c to add it. Thus prohibiting that space to be used for something
else.

> I think the problem is that once you
> start providing a facility extensions can use to store data along with the
> commit record then being restricted to 4 or 8 bytes is very limiting.

Well, you can easily use those 4/8 bytes to start adding more data to
the transaction. By referring to some table with transaction metadata
for example.

> It also doesn't allow you to load two extensions at once on a system.
> You wouldn't be able to have both the 'steve_commit_order' extension
> and BDR installed at the same time. I don't think this patch does a
> very good job at (B) but It wasn't intended to.

Well, I don't agree that steve_commit_order makes much sense in this
context. But there actually is a real problem here, namely that there's
no namespacing in those bytes. I'd be ok with saying that we split the
extradata in for bytes for the namespace and four for the actual
data. That's roughly how it works for advisory locks already.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 11/01/2014 09:00 PM, Michael Paquier wrote:
> 1) It is untested and actually there is no direct use for it in core.
> 2) Pushing code that we know as dead is no good, that's a feature more
> or less defined as maybe-useful-but-we-are-not-sure-yet-what-to-do-with-it.
> 3) If you're going to re-use this API in BDR, which is a fork of
> Postgres.

I would like to emphasise that BDR is not really a fork at all, no more
than any big topic branch would be a fork.

BDR is two major parts:

- A collection of patches to core (commit timestamps/extradata, sequence
AM, replication identifiers, logical decoding, DDL deparse, event
triggers, etc). These are being progressively submitted to core.
maintained as multiple feature branches plus a merged version; and

- An extension that uses core features and, where necessary, the
additions to core to implement bi-directional logical replication.

Because of the time scales involved in getting things into core it's
been necessary to *temporarily* get the 9.4-based feature branch into
wider use so that it can be used to run the BDR extension, but if we can
get the required features into core that need will go away.

Event triggers and logical decoding were already merged in 9.4.

If we can get things like commit timestamps, commit extradata / logical
replication identifiers, the sequence access method, etc merged in 9.5
then it should be possible to do away with the need for the patches to
core entirely and run BDR on top of stock 9.5. I'd be delighted if that
were possible, as doing away with the patched 9.4 would get rid of a
great deal of work and frustration on my part.

Note that the BDR extension its self is PostgreSQL-licensed. Externally
maintained extensions have been bought in-core before. It's a lot of
code though, and I can't imagine that being a quick process.

> You'd better complete this API in BDR by yourself and not
> bother core with that.

This argument would've prevented the inclusion of logical decoding,
which is rapidly becoming the headline feature for 9.4, or at least
shortly behind jsonb. Slony is being adapted to use it, multiple people
are working on auditing systems based on it, and AFAIK EDB's xDB is
being adapted to take advantage of it too.

As development gets more complex and people attempt bigger features, the
One Big Patch that adds a feature and an in-core user of the feature is
not going to be a viable approach all the time. In my view it's already
well past that, and some recent patches (like RLS) really should've been
split up into patch series.

If we want to avoid unreviewable monster-patches it will, IMO, be
necessary to have progressive, iterative enhancement. That may sometimes
mean that there's code in core that's only used by future
yet-to-be-merged patches and/or by extensions.

Of course its desirable to have an in-tree user of the code wherever
possible/practical - but sometimes it may *not* be possible or
practical. It seems to me like the benefits of committing work in
smaller, more reviewable chunks outweigh the benefits of merging
multiple related but separate changes just so everything can have an
immediate in-tree user.

That's not to say that extradata must remain glued to commit timestamps.
It might make more sense as a separate patch with an API to allow
extensions to manipulate it directly, plus a dummy extension showing how
it works, like we do with various hooks and with APIs like FDWs.
However, just like the various hooks that we have, it *does* make sense
to have something in-core that has no "real world" in-core users.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 06/11/14 08:50, Andres Freund wrote:
> On 2014-11-05 19:31:52 -0500, Steve Singer wrote:
>> It also doesn't allow you to load two extensions at once on a system.
>> You wouldn't be able to have both the 'steve_commit_order' extension
>> and BDR installed at the same time. I don't think this patch does a
>> very good job at (B) but It wasn't intended to.
>
> Well, I don't agree that steve_commit_order makes much sense in this
> context. But there actually is a real problem here, namely that there's
> no namespacing in those bytes. I'd be ok with saying that we split the
> extradata in for bytes for the namespace and four for the actual
> data. That's roughly how it works for advisory locks already.
>

I am not sure how would this solve problem of 2 extensions using the
extradata given that there can be only 1 record per txid anyway.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 06/11/14 01:31, Steve Singer wrote:
> On 11/05/2014 05:43 PM, Andres Freund wrote:
>
> Is this patch supposed to:
>
> A) Add commit timestamp tracking but nothing more
>
> B) Add infrastructure to store commit timestamps and provide a facility
> for storing additional bits of data extensions might want to be
> associated with the commit
>
> C). Add commit timestamps and node identifiers to commits
>
> If the answer is (A) then I can see why some people are objecting to
> adding extradata. If the answer is (B) then it's fair to ask how well
> does this patch handle various types of things people might want to
> attach to the commit record (such as the LSN). I think the problem is
> that once you start providing a facility extensions can use to store
> data along with the commit record then being restricted to 4 or 8 bytes
> is very limiting. It also doesn't allow you to load two extensions at
> once on a system. You wouldn't be able to have both the
> 'steve_commit_order' extension and BDR installed at the same time. I
> don't think this patch does a very good job at (B) but It wasn't
> intended to.
>

I would love to have (B) but I don't think that's realistic, at least
not in the extent some people on this thread would like. I mean you can
already do (B) by using table, it just isn't that great when it comes to
performance of that solution.

This patch is aimed to do limited version of (B) where you don't have
dynamic record for storing whatever you might desire but on the upside
the performance is good. And yes so far this look more like we are
actually doing (C) since main purpose of the patch is enabling conflict
detection and resolving of those conflicts, which is useful in many
replication scenarios that are not limited to the classical multi-master
setup.

> If what we are really doing is C, and just calling the space 'extradata'
> until we get the logical identifier stuff in and then we are going to
> rename extradata to nodeid then we should say so. If we are really
> concerned about the pg_upgrade impact of expanding the record later then
> maybe we should add 4 bytes of padding to the CommitTimeStampEntry now
> and but leave the manipulating the node id until later.
>

This might not be bad idea. I don't see the extradata being useful for
multiple extensions at the same time given that there is single record
per txid unless we would enforce some kind of limitation that extension
can only set the extradata for txids produced by that extension.

The namespacing idea that Andres has would again work fine for various
replication solutions as it would make it easier for them to coexist but
it would still not work for your 'steve_commit_order' (which I also
think should be done differently anyway).

In general I do see this patch to be similar in purpose to what we did
with replica triggers or logical decoding, those features also didn't
really have in-core use, were optional and enabled us to take step
forward with replication and had some side uses besides replication just
like commit timestamps do.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2014-11-07 17:54:32 +0100, Petr Jelinek wrote:
> On 06/11/14 08:50, Andres Freund wrote:
> >On 2014-11-05 19:31:52 -0500, Steve Singer wrote:
> >>It also doesn't allow you to load two extensions at once on a system.
> >>You wouldn't be able to have both the 'steve_commit_order' extension
> >>and BDR installed at the same time. I don't think this patch does a
> >>very good job at (B) but It wasn't intended to.
> >
> >Well, I don't agree that steve_commit_order makes much sense in this
> >context. But there actually is a real problem here, namely that there's
> >no namespacing in those bytes. I'd be ok with saying that we split the
> >extradata in for bytes for the namespace and four for the actual
> >data. That's roughly how it works for advisory locks already.
> >
>
> I am not sure how would this solve problem of 2 extensions using the
> extradata given that there can be only 1 record per txid anyway.

It'd help you to detect problems.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

> On Nov 5, 2014, at 7:31 PM, Steve Singer <steve(at)ssinger(dot)info> wrote:
>> On 11/05/2014 05:43 PM, Andres Freund wrote:
>> On 2014-11-05 17:17:05 -0500, Steve Singer wrote:
>> Imo that's essentially a different feature. What you essentially would
>> need here is a 'commit sequence number' - but no timestamps. And
>> probably to be useful that number has to be 8 bytes in itself.
>
> I think this gets to the heart of some of the differing views people have expressed on this patch
>
> Is this patch supposed to:
>
> A) Add commit timestamp tracking but nothing more
>
> B) Add infrastructure to store commit timestamps and provide a facility for storing additional bits of data extensions might want to be associated with the commit
>
> C). Add commit timestamps and node identifiers to commits

Well put.

I think the authors of this patch are suffering from a certain amount of myopia. Commit timestamps are useful, but so are commit LSNs, and it makes little sense to me to suppose that we should have two different systems for those closely-related needs.

Like Andres, I think B is impractical, so let's just be honest and admit that C is what we're really doing. But let's add LSNs so the people who want that can be happy too.

...Robert

On Nov 4, 2014, at 4:20 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 11/3/14 5:17 PM, Petr Jelinek wrote:
>>> Please don't name anything "committs". That looks like a misspelling of
>>> something.
>>>
>>> There is nothing wrong with
>>>
>>> pg_get_transaction_commit_timestamp()
>>>
>>> If you want to reduce the length, lose the "get".
>>
>> I am fine with that, I only wonder if your definition of "anything" only
>> concerns the SQL interfaces or also the internals.
>
> I'd be fine with commit_ts for internals, but not committs.

I agree that committs is poor. But I'd argue for spelling out commit_timestamp everywhere. It is more clear and easier to grep.

...Robert

On 08/11/14 00:35, Robert Haas wrote:
>> On Nov 5, 2014, at 7:31 PM, Steve Singer <steve(at)ssinger(dot)info> wrote:
>>> On 11/05/2014 05:43 PM, Andres Freund wrote:
>>> On 2014-11-05 17:17:05 -0500, Steve Singer wrote:
>>> Imo that's essentially a different feature. What you essentially would
>>> need here is a 'commit sequence number' - but no timestamps. And
>>> probably to be useful that number has to be 8 bytes in itself.
>>
>> I think this gets to the heart of some of the differing views people have expressed on this patch
>>
>> Is this patch supposed to:
>>
>> A) Add commit timestamp tracking but nothing more
>>
>> B) Add infrastructure to store commit timestamps and provide a facility for storing additional bits of data extensions might want to be associated with the commit
>>
>> C). Add commit timestamps and node identifiers to commits
>
> Well put.
>
> I think the authors of this patch are suffering from a certain amount of myopia. Commit timestamps are useful, but so are commit LSNs, and it makes little sense to me to suppose that we should have two different systems for those closely-related needs.
>
> Like Andres, I think B is impractical, so let's just be honest and admit that C is what we're really doing. But let's add LSNs so the people who want that can be happy too.
>

The list of what is useful might be long, but we can't have everything
there as there are space constraints, and LSN is another 8 bytes and I
still want to have some bytes for storing the "origin" or whatever you
want to call it there, as that's the one I personally have biggest
use-case for.
So this would be ~24bytes per txid already, hmm I wonder if we can pull
some tricks to lower that a bit.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Nov 7, 2014 at 7:07 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> The list of what is useful might be long,

That's FUD. It might also be short.

> but we can't have everything there
> as there are space constraints, and LSN is another 8 bytes and I still want
> to have some bytes for storing the "origin" or whatever you want to call it
> there, as that's the one I personally have biggest use-case for.
> So this would be ~24bytes per txid already, hmm I wonder if we can pull some
> tricks to lower that a bit.

It won't do to say "let's do the things that I want, and foreclose
forever the things that other people want". I find it quite hard to
believe that 16 bytes per transaction is a perfectly tolerable
overhead but 24 bytes per transaction will break the bank. But if
that is really true then we ought to reject this patch altogether,
because it's unacceptable, in any arena, for a patch that only
benefits extensions to consume all of the available bit-space in,
leaving none for future core needs.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 08/11/14 03:05, Robert Haas wrote:
> On Fri, Nov 7, 2014 at 7:07 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>> but we can't have everything there
>> as there are space constraints, and LSN is another 8 bytes and I still want
>> to have some bytes for storing the "origin" or whatever you want to call it
>> there, as that's the one I personally have biggest use-case for.
>> So this would be ~24bytes per txid already, hmm I wonder if we can pull some
>> tricks to lower that a bit.
>
> It won't do to say "let's do the things that I want, and foreclose
> forever the things that other people want". I find it quite hard to
> believe that 16 bytes per transaction is a perfectly tolerable
> overhead but 24 bytes per transaction will break the bank. But if
> that is really true then we ought to reject this patch altogether,
> because it's unacceptable, in any arena, for a patch that only
> benefits extensions to consume all of the available bit-space in,
> leaving none for future core needs.
>

That's not what I said. I am actually ok with adding the LSN if people
see it useful.
I was just wondering if we can make the record smaller somehow - 24bytes
per txid is around 96GB of data for whole txid range and won't work with
pages smaller than ~4kBs unless we add 6 char support to SLRU (which is
not hard and we could also not allow track_commit_timestamps to be
turned on with smaller pagesize...).

I remember somebody was worried about this already during the original
patch submission and it can't be completely ignored in the discussion
about adding more stuff into the record.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Nov 8, 2014 at 5:35 AM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> That's not what I said. I am actually ok with adding the LSN if people see
> it useful.
> I was just wondering if we can make the record smaller somehow - 24bytes per
> txid is around 96GB of data for whole txid range and won't work with pages
> smaller than ~4kBs unless we add 6 char support to SLRU (which is not hard
> and we could also not allow track_commit_timestamps to be turned on with
> smaller pagesize...).
>
> I remember somebody was worried about this already during the original patch
> submission and it can't be completely ignored in the discussion about adding
> more stuff into the record.

Fair point. Sorry I misunderstood.

I think the key question here is the time for which the data needs to
be retained. 2^32 of anything is a lot, but why keep around that
number of records rather than more (after all, we have epochs to
distinguish one use of a given txid from another) or fewer? Obvious
alternatives include:

- Keep the data for some period of time; discard the data when it's
older than some threshold.
- Keep a certain amount of total data; every time we create a new
file, discard the oldest one.
- Let consumers of the data say how much they need, and throw away
data when it's no longer needed by the oldest consumer.
- Some combination of the above.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11/07/2014 07:07 PM, Petr Jelinek wrote:

> The list of what is useful might be long, but we can't have everything
> there as there are space constraints, and LSN is another 8 bytes and I
> still want to have some bytes for storing the "origin" or whatever you
> want to call it there, as that's the one I personally have biggest
> use-case for.
> So this would be ~24bytes per txid already, hmm I wonder if we can
> pull some tricks to lower that a bit.
>

The reason why Jim and myself are asking for the LSN and not just the
timestamp is that I want to be able to order the transactions. Jim
pointed out earlier in the thread that just ordering on timestamp allows
for multiple transactions with the same timestamp.

Maybe we don't need the entire LSN to solve that. If you already have
the commit timestamp maybe you only need another byte or two of
granularity to order transactions that are within the same microsecond.

Robert Haas wrote:

> I think the key question here is the time for which the data needs to
> be retained. 2^32 of anything is a lot, but why keep around that
> number of records rather than more (after all, we have epochs to
> distinguish one use of a given txid from another) or fewer?

The problem is not how much data we retain; is about how much data we
can address. We must be able to address the data for transaction with
xid=2^32-1, even if you only retain the 1000 most recent transactions.
In fact, we already only retain data back to RecentXmin, if I recall
correctly. All slru.c users work that way.

Back when pg_multixact/members had the 5-char issue, I came up with a
patch that had each slru.c user declare how many chars maximum were the
filenames. I didn't push further with that because there was an issue
with it, I don't remember what it was offhand (but I don't think I
posted it). But this is only needed so that the filenames are all equal
width, which is mostly cosmetical; the rest of the module works fine
with 4- or 5-char filenames, and can be trivially expanded to support 6
or more.

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

On Sun, 2014-11-09 at 11:57 -0500, Steve Singer wrote:

> The reason why Jim and myself are asking for the LSN and not just the
> timestamp is that I want to be able to order the transactions. Jim
> pointed out earlier in the thread that just ordering on timestamp allows
> for multiple transactions with the same timestamp.
>
> Maybe we don't need the entire LSN to solve that. If you already have
> the commit timestamp maybe you only need another byte or two of
> granularity to order transactions that are within the same microsecond.

There is no guarantee that a commit with later LSN has a later
timestamp. There are cases where the clock could move significantly
backwards.

A robust solution to storing transaction commit information (including
commit order) in a way that can be referenced from other tables, can be
loaded to another cluster, and survives crashes would be a great
feature. But this feature doesn't have those properties.

- Anssi

On 10/11/14 08:01, Anssi Kääriäinen wrote:
> On Sun, 2014-11-09 at 11:57 -0500, Steve Singer wrote:
>
>> The reason why Jim and myself are asking for the LSN and not just the
>> timestamp is that I want to be able to order the transactions. Jim
>> pointed out earlier in the thread that just ordering on timestamp allows
>> for multiple transactions with the same timestamp.
>>
>> Maybe we don't need the entire LSN to solve that. If you already have
>> the commit timestamp maybe you only need another byte or two of
>> granularity to order transactions that are within the same microsecond.
>
> There is no guarantee that a commit with later LSN has a later
> timestamp. There are cases where the clock could move significantly
> backwards.
>
> A robust solution to storing transaction commit information (including
> commit order) in a way that can be referenced from other tables, can be
> loaded to another cluster, and survives crashes would be a great
> feature. But this feature doesn't have those properties.
>

It has the property of surviving crashes.

Not sure what you mean by referencing from other tables?

And about loading to another cluster, the txid does not really have any
meaning on another cluster, so the info about it does not have either?

But anyway this patch is targeting extensions not DBAs - you could write
extension that will provide that feature on top of this patch (although
given what I wrote above I don't see how it's useful).

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Nov 9, 2014 at 8:41 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> Robert Haas wrote:
>> I think the key question here is the time for which the data needs to
>> be retained. 2^32 of anything is a lot, but why keep around that
>> number of records rather than more (after all, we have epochs to
>> distinguish one use of a given txid from another) or fewer?
>
> The problem is not how much data we retain; is about how much data we
> can address.

I thought I was responding to a concern about disk space utilization.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Nov 10, 2014 at 2:01 AM, Anssi Kääriäinen
<anssi(dot)kaariainen(at)thl(dot)fi> wrote:
> There is no guarantee that a commit with later LSN has a later
> timestamp. There are cases where the clock could move significantly
> backwards.

Good point.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 09/11/14 17:57, Steve Singer wrote:
> On 11/07/2014 07:07 PM, Petr Jelinek wrote:
>> The list of what is useful might be long, but we can't have everything
>> there as there are space constraints, and LSN is another 8 bytes and I
>> still want to have some bytes for storing the "origin" or whatever you
>> want to call it there, as that's the one I personally have biggest
>> use-case for.
>> So this would be ~24bytes per txid already, hmm I wonder if we can
>> pull some tricks to lower that a bit.
>>
>
> The reason why Jim and myself are asking for the LSN and not just the
> timestamp is that I want to be able to order the transactions. Jim
> pointed out earlier in the thread that just ordering on timestamp allows
> for multiple transactions with the same timestamp.
>
> Maybe we don't need the entire LSN to solve that. If you already have
> the commit timestamp maybe you only need another byte or two of
> granularity to order transactions that are within the same microsecond.
>

Hmm maybe just one part of LSN, but I don't really like that either, if
we want to store LSN we should probably store it as is as somebody might
want to map it to txid for other reasons.

I did the calculation above wrong btw, it's actually 20 bytes not 24
bytes per record, I am inclined to just say we can live with that.

Since we agreed that the (B) case is not really feasible and we are
doing the (C), I also wonder if extradata should be renamed to nodeid
(even if it's not used at this point as nodeid). And then there is
question about the size of it, since the nodeid itself can live with 2
bytes probably ("64k of nodes ought to be enough for everybody" ;) ).
Or leave the extradata as is but use as reserved space for future use
and not expose it at this time on SQL level at all?

I guess Andres could answer what suits him better here.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Robert Haas wrote:
> On Sun, Nov 9, 2014 at 8:41 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> > Robert Haas wrote:
> >> I think the key question here is the time for which the data needs to
> >> be retained. 2^32 of anything is a lot, but why keep around that
> >> number of records rather than more (after all, we have epochs to
> >> distinguish one use of a given txid from another) or fewer?
> >
> > The problem is not how much data we retain; is about how much data we
> > can address.
>
> I thought I was responding to a concern about disk space utilization.

Ah, right. So AFAIK we don't need to keep anything older than
RecentXmin or something like that -- which is not too old. If I recall
correctly Josh Berkus was saying in a thread about pg_multixact that it
used about 128kB or so in <= 9.2 for his customers; that one was also
limited to RecentXmin AFAIR. I think a similar volume of commit_ts data
would be pretty acceptable. Moreso considering that it's turned off by
default.

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

On Mon, Nov 10, 2014 at 8:39 AM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> I did the calculation above wrong btw, it's actually 20 bytes not 24 bytes
> per record, I am inclined to just say we can live with that.

If you do it as 20 bytes, you'll have to do some work to squeeze out
the alignment padding. I'm inclined to think it's fine to have a few
extra padding bytes here; someone might want to use those for
something in the future, and they probably don't cost much.

> Since we agreed that the (B) case is not really feasible and we are doing
> the (C), I also wonder if extradata should be renamed to nodeid (even if
> it's not used at this point as nodeid). And then there is question about the
> size of it, since the nodeid itself can live with 2 bytes probably ("64k of
> nodes ought to be enough for everybody" ;) ).
> Or leave the extradata as is but use as reserved space for future use and
> not expose it at this time on SQL level at all?

I vote for calling it node-ID, and for allowing at least 4 bytes for
it. Penny wise, pound foolish.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Nov 10, 2014 at 8:40 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Ah, right. So AFAIK we don't need to keep anything older than
> RecentXmin or something like that -- which is not too old. If I recall
> correctly Josh Berkus was saying in a thread about pg_multixact that it
> used about 128kB or so in <= 9.2 for his customers; that one was also
> limited to RecentXmin AFAIR. I think a similar volume of commit_ts data
> would be pretty acceptable. Moreso considering that it's turned off by
> default.

I'm not sure whether keeping it just back to RecentXmin will be enough
for everybody's needs. But we certainly don't need to keep the last
2^32 records as someone-or-other was suggesting.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11/10/2014 08:39 AM, Petr Jelinek wrote:
> On 09/11/14 17:57, Steve Singer wrote:
>> On 11/07/2014 07:07 PM, Petr Jelinek wrote:
>>> The list of what is useful might be long, but we can't have everything
>>> there as there are space constraints, and LSN is another 8 bytes and I
>>> still want to have some bytes for storing the "origin" or whatever you
>>> want to call it there, as that's the one I personally have biggest
>>> use-case for.
>>> So this would be ~24bytes per txid already, hmm I wonder if we can
>>> pull some tricks to lower that a bit.
>>>
>>
>> The reason why Jim and myself are asking for the LSN and not just the
>> timestamp is that I want to be able to order the transactions. Jim
>> pointed out earlier in the thread that just ordering on timestamp allows
>> for multiple transactions with the same timestamp.
>>
>> Maybe we don't need the entire LSN to solve that. If you already have
>> the commit timestamp maybe you only need another byte or two of
>> granularity to order transactions that are within the same microsecond.
>>
>
> Hmm maybe just one part of LSN, but I don't really like that either,
> if we want to store LSN we should probably store it as is as somebody
> might want to map it to txid for other reasons.
>
> I did the calculation above wrong btw, it's actually 20 bytes not 24
> bytes per record, I am inclined to just say we can live with that.
>
> Since we agreed that the (B) case is not really feasible and we are
> doing the (C), I also wonder if extradata should be renamed to nodeid
> (even if it's not used at this point as nodeid). And then there is
> question about the size of it, since the nodeid itself can live with 2
> bytes probably ("64k of nodes ought to be enough for everybody" ;) ).
> Or leave the extradata as is but use as reserved space for future use
> and not expose it at this time on SQL level at all?
>
> I guess Andres could answer what suits him better here.
>

I am happy with renaming extradata to nodeid and not exposing it at this
time.

If we feel that commit-order (ie LSN or something equivalent) is really
a different patch/feature than commit-timestamp then I am okay with that
also but we should make sure to warn users of the commit-timestamp in
the documentation that two transactions might have the same timestamp
and that the commit order might not be the same as ordering by the
commit timestamp.

On 4 November 2014 08:23, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

>> 6) Shouldn't any value update of track_commit_timestamp be tracked in
>> XLogReportParameters? That's thinking about making the commit timestamp
>> available on standbys as well..
>
> Yes, it should.

Agree committs should be able to run on standby, but it seems possible
to do that without it running on the master. The two should be
unconnected.

Not sure why we'd want to have parameter changes on master reported?

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2014-11-11 16:10:47 +0000, Simon Riggs wrote:
> On 4 November 2014 08:23, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
> >> 6) Shouldn't any value update of track_commit_timestamp be tracked in
> >> XLogReportParameters? That's thinking about making the commit timestamp
> >> available on standbys as well..
> >
> > Yes, it should.
>
> Agree committs should be able to run on standby, but it seems possible
> to do that without it running on the master.

I don't think that's realistic. It requires WAL to be written in some
cases, so that's not going to work. I also don't think it's a
particularly interesting ability?

> The two should be unconnected.

Why?

> Not sure why we'd want to have parameter changes on master reported?

So it works correctly. We're currently truncating the slru on startup
when the guc is disabled which would cause problems WAL records coming
in from the primary. I think the code also needs some TLC to correctly
work after a failover.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 9 November 2014 16:57, Steve Singer <steve(at)ssinger(dot)info> wrote:
> On 11/07/2014 07:07 PM, Petr Jelinek wrote:
>
>
>> The list of what is useful might be long, but we can't have everything
>> there as there are space constraints, and LSN is another 8 bytes and I still
>> want to have some bytes for storing the "origin" or whatever you want to
>> call it there, as that's the one I personally have biggest use-case for.
>> So this would be ~24bytes per txid already, hmm I wonder if we can pull
>> some tricks to lower that a bit.
>>
>
> The reason why Jim and myself are asking for the LSN and not just the
> timestamp is that I want to be able to order the transactions. Jim pointed
> out earlier in the thread that just ordering on timestamp allows for
> multiple transactions with the same timestamp.
>
> Maybe we don't need the entire LSN to solve that. If you already have the
> commit timestamp maybe you only need another byte or two of granularity to
> order transactions that are within the same microsecond.

It looks like there are quite a few potential uses for this.

If we include everything it will be too fat to use for any of the
potential uses, since each will be pulled down by the others.

Sounds like it needs to be configurable in some way.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 11 November 2014 16:19, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-11-11 16:10:47 +0000, Simon Riggs wrote:
>> On 4 November 2014 08:23, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>
>> >> 6) Shouldn't any value update of track_commit_timestamp be tracked in
>> >> XLogReportParameters? That's thinking about making the commit timestamp
>> >> available on standbys as well..
>> >
>> > Yes, it should.
>>
>> Agree committs should be able to run on standby, but it seems possible
>> to do that without it running on the master.
>
> I don't think that's realistic. It requires WAL to be written in some
> cases, so that's not going to work. I also don't think it's a
> particularly interesting ability?

OK, so we are saying commit timestamp will NOT be available on Standbys.

I'm fine with that, since data changes aren't generated there.

> So it works correctly. We're currently truncating the slru on startup
> when the guc is disabled which would cause problems WAL records coming
> in from the primary. I think the code also needs some TLC to correctly
> work after a failover.

OK

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2014-11-11 17:09:54 +0000, Simon Riggs wrote:
> On 11 November 2014 16:19, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2014-11-11 16:10:47 +0000, Simon Riggs wrote:
> >> On 4 November 2014 08:23, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >>
> >> >> 6) Shouldn't any value update of track_commit_timestamp be tracked in
> >> >> XLogReportParameters? That's thinking about making the commit timestamp
> >> >> available on standbys as well..
> >> >
> >> > Yes, it should.
> >>
> >> Agree committs should be able to run on standby, but it seems possible
> >> to do that without it running on the master.
> >
> > I don't think that's realistic. It requires WAL to be written in some
> > cases, so that's not going to work. I also don't think it's a
> > particularly interesting ability?
>
> OK, so we are saying commit timestamp will NOT be available on Standbys.

Hm? They should be available - xact.c WAL replay will redo the setting
of the timestamps and explicitly overwritten timestamps will generate
their own WAL records. What I mean is just that you can't use commit
timestamps without also using it on the primary.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 11/10/14, 7:40 AM, Alvaro Herrera wrote:
> Robert Haas wrote:
>> On Sun, Nov 9, 2014 at 8:41 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>>> Robert Haas wrote:
>>>> I think the key question here is the time for which the data needs to
>>>> be retained. 2^32 of anything is a lot, but why keep around that
>>>> number of records rather than more (after all, we have epochs to
>>>> distinguish one use of a given txid from another) or fewer?
>>>
>>> The problem is not how much data we retain; is about how much data we
>>> can address.
>>
>> I thought I was responding to a concern about disk space utilization.
>
> Ah, right. So AFAIK we don't need to keep anything older than
> RecentXmin or something like that -- which is not too old. If I recall
> correctly Josh Berkus was saying in a thread about pg_multixact that it
> used about 128kB or so in <= 9.2 for his customers; that one was also
> limited to RecentXmin AFAIR. I think a similar volume of commit_ts data
> would be pretty acceptable. Moreso considering that it's turned off by
> default.

FWIW, AFAICS MultiXacts are only truncated after a (auto)vacuum process is able to advance datminmxid, which will (now) only happen when an entire relation has been scanned (which should be infrequent).

I believe the low normal space usage is just an indication that most databases don't use many MultiXacts.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

Jim Nasby wrote:
> On 11/10/14, 7:40 AM, Alvaro Herrera wrote:

> >Ah, right. So AFAIK we don't need to keep anything older than
> >RecentXmin or something like that -- which is not too old. If I recall
> >correctly Josh Berkus was saying in a thread about pg_multixact that it
> >used about 128kB or so in <= 9.2 for his customers; that one was also
> >limited to RecentXmin AFAIR. I think a similar volume of commit_ts data
> >would be pretty acceptable. Moreso considering that it's turned off by
> >default.
>
> FWIW, AFAICS MultiXacts are only truncated after a (auto)vacuum process is able to advance datminmxid, which will (now) only happen when an entire relation has been scanned (which should be infrequent).
>
> I believe the low normal space usage is just an indication that most databases don't use many MultiXacts.

That's in 9.3. Prior to that, they were truncated much more often.
Maybe you've not heard enough about this commit:

commit 0ac5ad5134f2769ccbaefec73844f8504c4d6182
Author: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Date: Wed Jan 23 12:04:59 2013 -0300

Improve concurrency of foreign key locking

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

On 11/11/14, 2:03 PM, Alvaro Herrera wrote:
> Jim Nasby wrote:
>> On 11/10/14, 7:40 AM, Alvaro Herrera wrote:
>
>>> Ah, right. So AFAIK we don't need to keep anything older than
>>> RecentXmin or something like that -- which is not too old. If I recall
>>> correctly Josh Berkus was saying in a thread about pg_multixact that it
>>> used about 128kB or so in <= 9.2 for his customers; that one was also
>>> limited to RecentXmin AFAIR. I think a similar volume of commit_ts data
>>> would be pretty acceptable. Moreso considering that it's turned off by
>>> default.
>>
>> FWIW, AFAICS MultiXacts are only truncated after a (auto)vacuum process is able to advance datminmxid, which will (now) only happen when an entire relation has been scanned (which should be infrequent).
>>
>> I believe the low normal space usage is just an indication that most databases don't use many MultiXacts.
>
> That's in 9.3. Prior to that, they were truncated much more often.

Well, we're talking about a new feature, so I wasn't looking in back branches. ;P

> Maybe you've not heard enough about this commit:
>
> commit 0ac5ad5134f2769ccbaefec73844f8504c4d6182

Interestingly, git.postgresql.org hasn't either: http://git.postgresql.org/gitweb/?p=postgresql.git&a=search&h=HEAD&st=commit&s=0ac5ad5134f2769ccbaefec73844f8504c4d6182

The commit is certainly there though...
decibel(at)decina:[15:12]~/pgsql/HEAD/src/backend (master=)$git log 0ac5ad5134f2769ccbaefec73844f8504c4d6182|head -n1
commit 0ac5ad5134f2769ccbaefec73844f8504c4d6182
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

Details below, but http://git.postgresql.org/gitweb/?p=postgresql.git&a=search&h=HEAD&st=commit&s=0ac5ad5134f2769ccbaefec73844f8504c4d6182 shows nothing, but that commit does exist:

decibel(at)decina:[15:12]~/pgsql/HEAD/src/backend (master=)$git log 0ac5ad5134f2769ccbaefec73844f8504c4d6182|head -n1
commit 0ac5ad5134f2769ccbaefec73844f8504c4d6182

Our github mirror doesn't show that commit in it's search either :(

-------- Original Message --------
Subject: Re: [HACKERS] tracking commit timestamps
Date: Tue, 11 Nov 2014 15:18:17 -0600
From: Jim Nasby <Jim(dot)Nasby(at)BlueTreble(dot)com>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
CC: Robert Haas <robertmhaas(at)gmail(dot)com>, Petr Jelinek <petr(at)2ndquadrant(dot)com>, Steve Singer <steve(at)ssinger(dot)info>, Andres Freund <andres(at)2ndquadrant(dot)com>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Anssi Kääriäinen <anssi(dot)kaariainen(at)thl(dot)fi>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>, "Pg Hackers" <pgsql-hackers(at)postgresql(dot)org>, Jaime Casanova <jaime(at)2ndquadrant(dot)com>

On 11/11/14, 2:03 PM, Alvaro Herrera wrote:
> Jim Nasby wrote:
>> On 11/10/14, 7:40 AM, Alvaro Herrera wrote:
>
>>> Ah, right. So AFAIK we don't need to keep anything older than
>>> RecentXmin or something like that -- which is not too old. If I recall
>>> correctly Josh Berkus was saying in a thread about pg_multixact that it
>>> used about 128kB or so in <= 9.2 for his customers; that one was also
>>> limited to RecentXmin AFAIR. I think a similar volume of commit_ts data
>>> would be pretty acceptable. Moreso considering that it's turned off by
>>> default.
>>
>> FWIW, AFAICS MultiXacts are only truncated after a (auto)vacuum process is able to advance datminmxid, which will (now) only happen when an entire relation has been scanned (which should be infrequent).
>>
>> I believe the low normal space usage is just an indication that most databases don't use many MultiXacts.
>
> That's in 9.3. Prior to that, they were truncated much more often.

Well, we're talking about a new feature, so I wasn't looking in back branches. ;P

> Maybe you've not heard enough about this commit:
>
> commit 0ac5ad5134f2769ccbaefec73844f8504c4d6182

Interestingly, git.postgresql.org hasn't either: http://git.postgresql.org/gitweb/?p=postgresql.git&a=search&h=HEAD&st=commit&s=0ac5ad5134f2769ccbaefec73844f8504c4d6182

The commit is certainly there though...
decibel(at)decina:[15:12]~/pgsql/HEAD/src/backend (master=)$git log 0ac5ad5134f2769ccbaefec73844f8504c4d6182|head -n1
commit 0ac5ad5134f2769ccbaefec73844f8504c4d6182
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Jim Nasby wrote:
> Details below, but http://git.postgresql.org/gitweb/?p=postgresql.git&a=search&h=HEAD&st=commit&s=0ac5ad5134f2769ccbaefec73844f8504c4d6182 shows nothing, but that commit does exist:
>
> decibel(at)decina:[15:12]~/pgsql/HEAD/src/backend (master=)$git log 0ac5ad5134f2769ccbaefec73844f8504c4d6182|head -n1
> commit 0ac5ad5134f2769ccbaefec73844f8504c4d6182
>
> Our github mirror doesn't show that commit in it's search either :(

No idea what "search" does, but it doesn't work on a commit ID. This
works:
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=0ac5ad5134f2769ccbaefec73844f8504c4d6182

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

Jim Nasby wrote:
> On 11/11/14, 2:03 PM, Alvaro Herrera wrote:
> >Jim Nasby wrote:
> >>On 11/10/14, 7:40 AM, Alvaro Herrera wrote:
> >
> >>>Ah, right. So AFAIK we don't need to keep anything older than
> >>>RecentXmin or something like that -- which is not too old. If I recall
> >>>correctly Josh Berkus was saying in a thread about pg_multixact that it
> >>>used about 128kB or so in <= 9.2 for his customers; that one was also
> >>>limited to RecentXmin AFAIR. I think a similar volume of commit_ts data
> >>>would be pretty acceptable. Moreso considering that it's turned off by
> >>>default.
> >>
> >>FWIW, AFAICS MultiXacts are only truncated after a (auto)vacuum process is able to advance datminmxid, which will (now) only happen when an entire relation has been scanned (which should be infrequent).
> >>
> >>I believe the low normal space usage is just an indication that most databases don't use many MultiXacts.
> >
> >That's in 9.3. Prior to that, they were truncated much more often.
>
> Well, we're talking about a new feature, so I wasn't looking in back branches. ;P

Well, I did mention <= 9.2 in the text above ...

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

On 11/11/14, 3:55 PM, Alvaro Herrera wrote:
> Jim Nasby wrote:
>> Details below, but http://git.postgresql.org/gitweb/?p=postgresql.git&a=search&h=HEAD&st=commit&s=0ac5ad5134f2769ccbaefec73844f8504c4d6182 shows nothing, but that commit does exist:
>>
>> decibel(at)decina:[15:12]~/pgsql/HEAD/src/backend (master=)$git log 0ac5ad5134f2769ccbaefec73844f8504c4d6182|head -n1
>> commit 0ac5ad5134f2769ccbaefec73844f8504c4d6182
>>
>> Our github mirror doesn't show that commit in it's search either :(
>
> No idea what "search" does, but it doesn't work on a commit ID. This
> works:
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=0ac5ad5134f2769ccbaefec73844f8504c4d6182

Well, this is rather confusing, because the drop-down by the search box on [1] has a selection for "commit". You'd think that would allow you to search for a specific commit.

Turns out, the help [2] states that the "commit" context for search searches commit author and messages. So I guess it's as expected, albeit confusing. :(

Anyone know how hard it would be to allow a commit "search" to also look for a specific commit hash?

1: http://git.postgresql.org/gitweb/?p=postgresql.git;a=summary
2: http://git.postgresql.org/gitweb/?p=postgresql.git;a=search_help
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Wed, Nov 12, 2014 at 12:08 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> On 11/11/14, 3:55 PM, Alvaro Herrera wrote:
>>
>> Jim Nasby wrote:
>>>
>>> Details below, but
>>> http://git.postgresql.org/gitweb/?p=postgresql.git&a=search&h=HEAD&st=commit&s=0ac5ad5134f2769ccbaefec73844f8504c4d6182
>>> shows nothing, but that commit does exist:
>>>
>>> decibel(at)decina:[15:12]~/pgsql/HEAD/src/backend (master=)$git log
>>> 0ac5ad5134f2769ccbaefec73844f8504c4d6182|head -n1
>>> commit 0ac5ad5134f2769ccbaefec73844f8504c4d6182
>>>
>>> Our github mirror doesn't show that commit in it's search either :(
>>
>>
>> No idea what "search" does, but it doesn't work on a commit ID. This
>> works:
>>
>> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=0ac5ad5134f2769ccbaefec73844f8504c4d6182
>
>
> Well, this is rather confusing, because the drop-down by the search box on
> [1] has a selection for "commit". You'd think that would allow you to search
> for a specific commit.
>
> Turns out, the help [2] states that the "commit" context for search searches
> commit author and messages. So I guess it's as expected, albeit confusing.
> :(
>
> Anyone know how hard it would be to allow a commit "search" to also look for
> a specific commit hash?

Preferably submit it for inclusion *upstream* as a feature. We'd
rather not end up forking gitweb.

(And while at it, feel free to fix it to be less super-slow :P)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On 10/11/14 14:53, Robert Haas wrote:
> On Mon, Nov 10, 2014 at 8:39 AM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>> I did the calculation above wrong btw, it's actually 20 bytes not 24 bytes
>> per record, I am inclined to just say we can live with that.
>
> If you do it as 20 bytes, you'll have to do some work to squeeze out
> the alignment padding. I'm inclined to think it's fine to have a few
> extra padding bytes here; someone might want to use those for
> something in the future, and they probably don't cost much.
>

I did get around the alignment via memcpy, so it is still 20bytes.

>> Since we agreed that the (B) case is not really feasible and we are doing
>> the (C), I also wonder if extradata should be renamed to nodeid (even if
>> it's not used at this point as nodeid). And then there is question about the
>> size of it, since the nodeid itself can live with 2 bytes probably ("64k of
>> nodes ought to be enough for everybody" ;) ).
>> Or leave the extradata as is but use as reserved space for future use and
>> not expose it at this time on SQL level at all?
>
> I vote for calling it node-ID, and for allowing at least 4 bytes for
> it. Penny wise, pound foolish.
>

Ok, I went this way.

Anyway here is v8 version of the patch, I think I addressed all the
concerns mentioned, it's also rebased against current master (BRIN
commit added some conflicts).

Brief list of changes:
- the commit timestamp record now stores timestamp, lsn and nodeid
- added code to disallow turning track_commit_timestamp on with too
small pagesize
- the get interfaces error out when track_commit_timestamp is off
- if the xid passed to get interface is out of range -infinity
timestamp is returned (I think it's bad idea to throw errors here as the
valid range is not static and same ID can start throwing errors between
calls theoretically)
- renamed the sql interfaces to pg_xact_commit_timestamp,
pg_xact_commit_timestamp_data and pg_last_committed_xact, they don't
expose the nodeid atm, I personally am not big fan of the "xact" but it
seems more consistent with existing naming
- documented pg_resetxlog changes and make all the pg_resetxlog
options alphabetically ordered
- committs is not used anymore, it's commit_ts (and CommitTs in
camelcase), I think it's not really good idea to spell the timestamp
everywhere as some interface then get 30+ chars long names...
- added WAL logging of the track_commit_timestamp GUC
- added alternative expected output of the regression test so that it
works with make installcheck when track_commit_timestamp is on
- added C interface to set default nodeid for current backend
- several minor comment and naming adjustments mostly suggested by Michael

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 11/12/14, 7:06 AM, Petr Jelinek wrote:
> - if the xid passed to get interface is out of range -infinity timestamp is returned (I think it's bad idea to throw errors here as the valid range is not static and same ID can start throwing errors between calls theoretically)

Wouldn't NULL be more appropriate?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Thu, Nov 13, 2014 at 7:56 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> On 11/12/14, 7:06 AM, Petr Jelinek wrote:
>>
>> - if the xid passed to get interface is out of range -infinity timestamp
>> is returned (I think it's bad idea to throw errors here as the valid range
>> is not static and same ID can start throwing errors between calls
>> theoretically)
>
>
> Wouldn't NULL be more appropriate?
Definitely. Defining a given value for information not valid is awkward.
--
Michael

On Wed, Nov 12, 2014 at 10:06 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> Brief list of changes:
> - the commit timestamp record now stores timestamp, lsn and nodeid
Now that not only the commit timestamp is stored, calling that "commit
timestamp", "committs" or "commit_timestamp" is strange, no? If this
patch is moving toward being a more complex information provider,
calling it commit information or commit data is more adapted, no?
Documentation would need a fresh brush as well in this case.

> - added code to disallow turning track_commit_timestamp on with too small
> pagesize
> - the get interfaces error out when track_commit_timestamp is off
OK, that's sane.

> - if the xid passed to get interface is out of range -infinity timestamp is
> returned (I think it's bad idea to throw errors here as the valid range is
> not static and same ID can start throwing errors between calls
> theoretically)
Already mentioned by Jim in a previous mail: this would be better as NULL.

> - renamed the sql interfaces to pg_xact_commit_timestamp,
> pg_xact_commit_timestamp_data and pg_last_committed_xact, they don't expose
> the nodeid atm, I personally am not big fan of the "xact" but it seems more
> consistent with existing naming
pg_xact_commit_timestamp and pg_xact_commit_timestamp_data are
overlapping. What's wrong with a single function able to return the
whole set (node ID, commit timetamp, commit LSN)? Let's say
pg_xact_commit_information or pg_xact_commit_data. Already mentioned,
but I also find using a SRF able to return all the available
information from a given XID value quite useful. And this does not
conflict with what is proposed currently, you would need just to call
the function with XID + number of entries wanted to get a single one.
Comments from other folks about that?

> - documented pg_resetxlog changes and make all the pg_resetxlog options
> alphabetically ordered
> - added WAL logging of the track_commit_timestamp GUC
> - added alternative expected output of the regression test so that it works
> with make installcheck when track_commit_timestamp is on
> - added C interface to set default nodeid for current backend
> - several minor comment and naming adjustments mostly suggested by Michael
Thanks for those adjustments.

Then more input about the latest patch:
1) This block is not needed, option -e is listed twice:
The <option>-o</>, <option>-x</>, <option>-e</>,
- <option>-m</>, <option>-O</>,
- and <option>-l</>
+ <option>-m</>, <option>-O</>, <option>-l</>
+ and <option>-e</>
2) Very small thing: a couple of files have no newlines at the end,
among them committs.conf and test_committs/Makefile.
3) pg_last_committed_xact and not pg_last_xact_commit_information or similar?
4) storage.sgml needs to be updated with the new folder pg_committs
5) Er.. node ID is missing in pg_last_committed_xact, no?
6) This XXX notice can be removed:
+ /*
+ * Return empty if the requested value is older than what we have or
+ * newer than newest we have.
+ *
+ * XXX: should this be error instead?
+ */
We are moving toward returning invalid information in the SQL
interface when the information is not in history instead of an error,
no? (Note that I am still a partisan of an error message to let the
caller know that commit info history does not have the information
requested).
7) Note that TransactionTreeSetCommitTsData still never sets do_xlog
at true and that WriteSetTimestampXlogRec never gets called. So no
information is WAL-logged with this patch. Wouldn't that be useful for
standbys as well? Perhaps I am missing why this is disabled? This code
should be activated IMO or it would be just untested.
8) As a more general point, the node ID stuff makes me uncomfortable
and is just added on top of the existing patch without much
thinking... So I am really skeptical about it. The need here is to
pass on demand a int8 that is a node ID that can only be set through a
C interface, so only extensions could play with it. The data passed to
a WAL record is always built and determined by the system and entirely
transparent to the user, inserting user-defined data like that
inconsistent with what we've been doing until now, no?

Also, a question particularly for BDR and Slony folks: do you
sometimes add a new node using the base backup of an existing node :)
See what I come up with: a duplication of this new node ID system with
the already present system ID, no?
Similarly, the LSN is added to the WAL record containing the commit
timestamp, but cannot the LSN of the WAL record containing the commit
timestamp itself be used as a point of reference for a better
ordering? That's not exactly the same as the LSN of the transaction
commit, still it provides a WAL-based reference.
Regards,
--
Michael

On 13/11/14 07:04, Michael Paquier wrote:
> On Wed, Nov 12, 2014 at 10:06 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>> Brief list of changes:
>> - the commit timestamp record now stores timestamp, lsn and nodeid
> Now that not only the commit timestamp is stored, calling that "commit
> timestamp", "committs" or "commit_timestamp" is strange, no? If this
> patch is moving toward being a more complex information provider,
> calling it commit information or commit data is more adapted, no?

It's not, since adding more info will break upgrades, I doubt we will
add more anytime soon. I was thinking about it too tbh, but don't have
better name (I don't like commit data as it seems confusing - isn't
commit data the dml you just committed?).
Maybe commit_metadata, commit_information is probably ok also, this
would need input from others, I am personally fine with keeping the
commit_timestamp name too.

>
>> - if the xid passed to get interface is out of range -infinity timestamp is
>> returned (I think it's bad idea to throw errors here as the valid range is
>> not static and same ID can start throwing errors between calls
>> theoretically)
> Already mentioned by Jim in a previous mail: this would be better as NULL.

Yeah that make sense, I have no idea what I was thinking :)

>
>> - renamed the sql interfaces to pg_xact_commit_timestamp,
>> pg_xact_commit_timestamp_data and pg_last_committed_xact, they don't expose
>> the nodeid atm, I personally am not big fan of the "xact" but it seems more
>> consistent with existing naming
> pg_xact_commit_timestamp and pg_xact_commit_timestamp_data are
> overlapping. What's wrong with a single function able to return the
> whole set (node ID, commit timetamp, commit LSN)? Let's say

That's what pg_xact_commit_timestamp_data does (it does not show nodeid
because we agreed that it should not be exposed yet on sql level). Might
make sense to rename, but let's wait for input about the general
renaming point at the beginning of the mail.

> pg_xact_commit_information or pg_xact_commit_data. Already mentioned,
> but I also find using a SRF able to return all the available
> information from a given XID value quite useful. And this does not
> conflict with what is proposed currently, you would need just to call
> the function with XID + number of entries wanted to get a single one.
> Comments from other folks about that?

No idea what you mean by this to be honest, there is exactly one record
stored for single XID.

>
> Then more input about the latest patch:
> 1) This block is not needed, option -e is listed twice:
> The <option>-o</>, <option>-x</>, <option>-e</>,
> - <option>-m</>, <option>-O</>,
> - and <option>-l</>
> + <option>-m</>, <option>-O</>, <option>-l</>
> + and <option>-e</>
> 2) Very small thing: a couple of files have no newlines at the end,
> among them committs.conf and test_committs/Makefile.
> 3) pg_last_committed_xact and not pg_last_xact_commit_information or similar?

Just inspiration from DB2's rollforward (which shows among other things
"last committed transaction: <timestamp>"), but I don't feel strongly
about naming so can be changed.

> 4) storage.sgml needs to be updated with the new folder pg_committs

Right.

> 5) Er.. node ID is missing in pg_last_committed_xact, no?

That's intentional (for now).

> 6) This XXX notice can be removed:
> + /*
> + * Return empty if the requested value is older than what we have or
> + * newer than newest we have.
> + *
> + * XXX: should this be error instead?
> + */

Ok.

> (Note that I am still a partisan of an error message to let the
> caller know that commit info history does not have the information
> requested).

IMHO throwing error there would be same as throwing error when WHERE
clause in SELECT does not match anything. As the xid range for which we
store data is dynamic we need to accept any xid as valid input because
the caller has no way of validating if the xid passed is out of range or
not.

> 7) Note that TransactionTreeSetCommitTsData still never sets do_xlog
> at true and that WriteSetTimestampXlogRec never gets called. So no
> information is WAL-logged with this patch. Wouldn't that be useful for
> standbys as well? Perhaps I am missing why this is disabled? This code
> should be activated IMO or it would be just untested.

True is only needed here when you are setting this info to different
transaction than the one you are in since the info can be reconstructed
from normal transaction WAL record (see that it's actually called from
xact_redo_commit_internal, which is how we get the WAL safety and why it
works on slave). So the true is for use by extensions only, it's not
completely uncommon that we have APIs that are used only by extensions.

> 8) As a more general point, the node ID stuff makes me uncomfortable
> and is just added on top of the existing patch without much
> thinking... So I am really skeptical about it. The need here is to
> pass on demand a int8 that is a node ID that can only be set through a
> C interface, so only extensions could play with it. The data passed to
> a WAL record is always built and determined by the system and entirely
> transparent to the user, inserting user-defined data like that
> inconsistent with what we've been doing until now, no?

Again it's not exposed to SQL because I thought there was agreement to
not do that yet since we might want to build some more core stuff on top
of that before exposing it. It's part of the record now because it can
be useful already the way it is and because adding it later would break
pg_upgrade (it's int4 btw).

Also I would really not say it was added without thought, I am one of
the BDR developers and I was before one of the Londiste developers so I
did think about what I would want when in those shoes.

That being said I think I will remove the CommitTsSetDefaultNodeId
interface in next revision, as extension can already set nodeid via
TransactionTreeSetCommitTsData call and we might want to revisit the
CommitTsSetDefaultNodeId stuff once we start implementing the
replication identifiers. Not to mention that I realized in meantime that
CommitTsSetDefaultNodeId the way it's done currently isn't crash safe
(it's not hard to make it crash safe). And since it's quite simple we
can add it at later date easily if needed.

>
> Also, a question particularly for BDR and Slony folks: do you
> sometimes add a new node using the base backup of an existing node :)
> See what I come up with: a duplication of this new node ID system with
> the already present system ID, no?

Yes we do use basebackup sometimes and no it's not possible to use
systemid here:
- the point of nodeid is to be able to store *remote* nodeid as well
as local one (depending where the change actually originated from) so
your local systemid is quite useless there
- systemid is per Postgres instance, you need per-db identifier when
doing logical rep (2 dbs can have single db as destination or the other
way around)

> Similarly, the LSN is added to the WAL record containing the commit
> timestamp, but cannot the LSN of the WAL record containing the commit
> timestamp itself be used as a point of reference for a better
> ordering? That's not exactly the same as the LSN of the transaction
> commit, still it provides a WAL-based reference.

No, again for several reasons:
- as you pointed out yourself the LSN might not be same as LSN for the xid
- more importantly we normally don't do special WAL logging for commit
timestamp

How it works is that because currently the
TransactionTreeSetCommitTsData is always called with xid of the current
transaction, the WAL record for commit of current transaction can be
used to get the info we need (both timestamp and lsn are used in fact).
As I said above, see how TransactionTreeSetCommitTsData is called from
xact_redo_commit_internal.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 9 November 2014 16:57, Steve Singer <steve(at)ssinger(dot)info> wrote:
> On 11/07/2014 07:07 PM, Petr Jelinek wrote:
>
>
>> The list of what is useful might be long, but we can't have everything
>> there as there are space constraints, and LSN is another 8 bytes and I still
>> want to have some bytes for storing the "origin" or whatever you want to
>> call it there, as that's the one I personally have biggest use-case for.
>> So this would be ~24bytes per txid already, hmm I wonder if we can pull
>> some tricks to lower that a bit.
>>
>
> The reason why Jim and myself are asking for the LSN and not just the
> timestamp is that I want to be able to order the transactions. Jim pointed
> out earlier in the thread that just ordering on timestamp allows for
> multiple transactions with the same timestamp.

I think we need to be clear about the role and function of components here.

Xid timestamps allow a replication system to do post-commit conflict
resolution based upon timestamp, i.e. last update wins. That is
potentially usable by BDR, Slony, xdb and anything else that wants
that.

Ordering transactions in LSN order is very precisly the remit of the
existing logical decoding API. Any user that wishes to see a commits
in sequence can do so using that API. BDR already does this, as do
other users of the decoding API. So Slony already has access to a
useful ordering if it wishes it. We do not need to anything *on this
patch* to enable LSNs for Slony or anyone else. I don't see any reason
to provide the same facility twice, in two different ways.

So in summary... the components are
* Commit LSN order is useful for applying changes - available by
logical decoding
* Commit timestamps and nodeid are useful for conflict resolution -
available from this patch

Both components have been designed in ways that allow multiple
replication systems to use these facilities.

So, -1 to including commit LSN in the SLRU alongside commit timestamp
and nodeid.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 13/11/14 14:18, Simon Riggs wrote:
>
> So in summary... the components are
> * Commit LSN order is useful for applying changes - available by
> logical decoding
> * Commit timestamps and nodeid are useful for conflict resolution -
> available from this patch
>
> Both components have been designed in ways that allow multiple
> replication systems to use these facilities.
>
> So, -1 to including commit LSN in the SLRU alongside commit timestamp
> and nodeid.
>

I am of the same opinion, I added the LSN "by popular demand", but I
still personally don't see the value in having it there as it does *not*
enable us to do something that would be impossible otherwise.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Nov 13, 2014 at 8:18 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Ordering transactions in LSN order is very precisly the remit of the
> existing logical decoding API. Any user that wishes to see a commits
> in sequence can do so using that API. BDR already does this, as do
> other users of the decoding API. So Slony already has access to a
> useful ordering if it wishes it. We do not need to anything *on this
> patch* to enable LSNs for Slony or anyone else. I don't see any reason
> to provide the same facility twice, in two different ways.

Perhaps you could respond more specifically to concerns expressed
upthread, like:

http://www.postgresql.org/message-id/BLU436-SMTP28B68B9312CBE5D9125441DC870@phx.gbl

I don't see that as a dumb argument on the face of it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 13 November 2014 21:24, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Nov 13, 2014 at 8:18 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Ordering transactions in LSN order is very precisly the remit of the
>> existing logical decoding API. Any user that wishes to see a commits
>> in sequence can do so using that API. BDR already does this, as do
>> other users of the decoding API. So Slony already has access to a
>> useful ordering if it wishes it. We do not need to anything *on this
>> patch* to enable LSNs for Slony or anyone else. I don't see any reason
>> to provide the same facility twice, in two different ways.
>
> Perhaps you could respond more specifically to concerns expressed
> upthread, like:
>
> http://www.postgresql.org/message-id/BLU436-SMTP28B68B9312CBE5D9125441DC870@phx.gbl
>
> I don't see that as a dumb argument on the face of it.

We have a clear "must have" argument for timestamps to support
replication conflicts.

Adding LSNs, when we already have a way to access them, is much more
of a nice to have. I don't really see it as a particularly nice to
have either, since the SLRU is in no way ordered.

Scope creep is a dangerous thing, so we shouldn't, and elsewhere
don't, collect up ideas like a bag of mixed sweets. It's easy to
overload things to the point where they don't fly at all. The whole
point of this is that we're building something faster than trigger
based systems.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Nov 13, 2014 at 6:55 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 13 November 2014 21:24, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Nov 13, 2014 at 8:18 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> Ordering transactions in LSN order is very precisly the remit of the
>>> existing logical decoding API. Any user that wishes to see a commits
>>> in sequence can do so using that API. BDR already does this, as do
>>> other users of the decoding API. So Slony already has access to a
>>> useful ordering if it wishes it. We do not need to anything *on this
>>> patch* to enable LSNs for Slony or anyone else. I don't see any reason
>>> to provide the same facility twice, in two different ways.
>>
>> Perhaps you could respond more specifically to concerns expressed
>> upthread, like:
>>
>> http://www.postgresql.org/message-id/BLU436-SMTP28B68B9312CBE5D9125441DC870@phx.gbl
>>
>> I don't see that as a dumb argument on the face of it.
>
> We have a clear "must have" argument for timestamps to support
> replication conflicts.
>
> Adding LSNs, when we already have a way to access them, is much more
> of a nice to have. I don't really see it as a particularly nice to
> have either, since the SLRU is in no way ordered.
>
> Scope creep is a dangerous thing, so we shouldn't, and elsewhere
> don't, collect up ideas like a bag of mixed sweets. It's easy to
> overload things to the point where they don't fly at all. The whole
> point of this is that we're building something faster than trigger
> based systems.

I think that's slamming the door closed and nailing it shut behind
you. If we add the feature without LSNs, how will someone go back and
add that later? It would change the on-disk format of the SLRU, so to
avoid breaking pg_upgrade, someone would have to write a conversion
utility. Even at that, it would slow pg_upgrade down when the feature
has been used.

By way of contrast, adding the feature now is quite easy. It just
requires storing a few more bytes per transaction.

I am all in favor of incremental development when possible, but not
when it so greatly magnifies the work that needs to be done. People
have been asking for the ability to determine the commit ordering for
years, and this is a way to do that, very inexpensively, as part of a
patch that is needed anyway. We are not talking about loading 20 new
requirements on top of this patch; that would be intolerable. We're
talking about adding one additional piece of information that has been
requested multiple times over the years.

The way I see it, there are at least three uses for this information.
One, trigger-based replication solutions. While logical decoding will
doubtless be preferable, I don't think trigger-based replication
solutions will go away completely, and certainly not right away.
They've wanted this since forever. Two, for user applications that
want to know the commit order for their own purposes, as in Steve's
example. Three, for O(1) snapshots. Heikki's patch to make that
happen seems to have stalled a bit, but if it's ever to go anywhere it
will need something like this.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 14 November 2014 17:12, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> We are not talking about loading 20 new
> requirements on top of this patch; that would be intolerable. We're
> talking about adding one additional piece of information that has been
> requested multiple times over the years.

The requested information is already available, as discussed. Logical
decoding adds commit ordering for *exactly* the purpose of using it
for replication, available to all solutions. This often requested
feature has now been added and doesn't need to be added twice.

So what we are discussing is adding a completely superfluous piece of
information.

Not including the LSN info does nothing to trigger based replication,
which will no doubt live on happily for many years. But adding LSN
will slow down logical replication, for no purpose at all.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 11/14/2014 08:21 PM, Simon Riggs wrote:
> The requested information is already available, as discussed. Logical
> decoding adds commit ordering for *exactly* the purpose of using it
> for replication, available to all solutions. This often requested
> feature has now been added and doesn't need to be added twice.
>
> So what we are discussing is adding a completely superfluous piece of
> information.
>
> Not including the LSN info does nothing to trigger based replication,
> which will no doubt live on happily for many years. But adding LSN
> will slow down logical replication, for no purpose at all.
>

Simon,
The use cases I'm talking about aren't really replication related. Often
I have come across systems that want to do something such as 'select *
from orders where X > the_last_row_I_saw order by X' and then do further
processing on the order.

This is kind of awkard to do today because you don't have a good
candidate for 'X' to order on. Using either a sequence or insert-row
timestamp doesn't work well because a transaction with a lower value for
X might end up committing after the higher value in in a query result.

Yes you could setup a logical wal slot and listen on the stream of
inserts into your order table but thats a lot of administration overhead
compared to just issuing an SQL query for what really is a query type
operation.

Using the commit timestamp for my X sounded very tempting but could
allow duplicates.

One could argue that this patch is about replication features, and
providing commit ordering for query purposes should be a separate patch
to add that on top of this infrastructure. I see merit to smaller more
focused patches but that requires leaving the door open to easily
extending things later.

It could also be that I'm the only one who wants to order and filter
queries in this manner (but that would surprise me). If the commit lsn
has limited appeal and we decide we don't want it at all then we
shouldn't add it. I've seen this type of requirement in a number of
different systems at a number of different companies. I've generally
seen it dealt with by either selecting rows behind the last now()
timestamp seen and then filtering out already processed rows or by
tracking the 'processed' state of each row individually (ie performing
an update on each row once its been processed) which performs poorly.

Steve

On 15 November 2014 04:32, Steve Singer <steve(at)ssinger(dot)info> wrote:

> The use cases I'm talking about aren't really replication related. Often I
> have come across systems that want to do something such as 'select * from
> orders where X > the_last_row_I_saw order by X' and then do further
> processing on the order.

Yes, existing facilities provide mechanisms for different types of
application change queues.

If you want to write a processing queue in SQL, that isn't the best
way. You'll need some way to keep track of whether or not its been
successfully processed. That's either a column in the table, or a
column in a queue table maintained by triggers, with the row write
locked on read. You can then have multiple readers from this queue
using the new SKIP LOCKED feature, which was specifically designed to
facilitate that.

Logical decoding was intended for much more than just replication. It
provides commit order access to changed data in a form that is both
usable and efficient for high volume applicatiion needs.

I don't see any reason to add LSN into a SLRU updated at commit to
support those application needs.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 15/11/14 13:36, Simon Riggs wrote:
> On 15 November 2014 04:32, Steve Singer <steve(at)ssinger(dot)info> wrote:
>
>> The use cases I'm talking about aren't really replication related. Often I
>> have come across systems that want to do something such as 'select * from
>> orders where X > the_last_row_I_saw order by X' and then do further
>> processing on the order.
>
> Yes, existing facilities provide mechanisms for different types of
> application change queues.
>
> If you want to write a processing queue in SQL, that isn't the best
> way. You'll need some way to keep track of whether or not its been
> successfully processed. That's either a column in the table, or a
> column in a queue table maintained by triggers, with the row write
> locked on read. You can then have multiple readers from this queue
> using the new SKIP LOCKED feature, which was specifically designed to
> facilitate that.
>
> Logical decoding was intended for much more than just replication. It
> provides commit order access to changed data in a form that is both
> usable and efficient for high volume applicatiion needs.
>
> I don't see any reason to add LSN into a SLRU updated at commit to
> support those application needs.
>

I am still on the fence about the LSN issue, I don't mind it from code
perspective, it's already written anyway, but I am not sure if we really
want it in the SLRU as Simon says.

Mainly because of three things:
One, this patch is not really feature patch, as you can do most of what
it does via tables already, but more a performance improvement and we
should try to make it perform as good as possible then, adding more
things does not really improve performance (according to my benchmarks
the performance difference with/without LSN is under 1% so it's not
terrible, but it's there), not to mention additional disk space.

Two, the LSN use-cases seem to still be only theoretical to me, while
the timestamp use-case has been production problem for at least a decade.