pgsql: Keep track of transaction commit timestamps

On 12/03/2014 03:54 PM, Alvaro Herrera wrote:

Keep track of transaction commit timestamps

Transactions can now set their commit timestamp directly as they commit,
or an external transaction commit timestamp can be fed from an outside
system using the new function TransactionTreeSetCommitTsData(). This
data is crash-safe, and truncated at Xid freeze point, same as pg_clog.

This module is disabled by default because it causes a performance hit,
but can be enabled in postgresql.conf requiring only a server restart.

A new test in src/test/modules is included.

Catalog version bumped due to the new subdirectory within PGDATA and a
couple of new SQL functions.

Authors: Álvaro Herrera and Petr Jelínek

Reviewed to varying degrees by Michael Paquier, Andres Freund, Robert
Haas, Amit Kapila, Fujii Masao, Jaime Casanova, Simon Riggs, Steven
Singer, Peter Eisentraut

this broke the docs build:

http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=guaibasaurus&dt=2014-12-03%2016%3A17%3A01

Stefan

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

Stefan Kaltenbrunner wrote:

On 12/03/2014 03:54 PM, Alvaro Herrera wrote:

Keep track of transaction commit timestamps

this broke the docs build:

http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=guaibasaurus&dt=2014-12-03%2016%3A17%3A01

Woops, thanks. Fixed.

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

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

On 12/03/2014 04:54 PM, Alvaro Herrera wrote:

ir commit timestamp directly as they commit,
or an external transaction c

Sorry, I'm late to the party, but here's some random comments on this
after a quick review:

* The whole concept of a node ID seems to be undocumented, and unused.
No-one calls CommitTsSetDefaultNodeId(). The whole SET_TS record type is
dead code too, when a non-default node_id is not set. Please remove, or
explain how all that's supposed to work.

* COMMIT_TS_SETTS. "SETTS" sounds like a typo (like Robert complained
about "committs" earlier). Rename to "COMMIT_TS_SET_TIMESTAMP", or just
"COMMIT_TS_SET".

* committsdesc.c -> commit_ts_desc.c, to be consistent with "commit_ts.c"

* In commit_ts_desc:

+       nsubxids = ((XLogRecGetDataLen(record) - SizeOfCommitTsSet) /
+                   sizeof(TransactionId));
+       if (nsubxids > 0)
+       {
+           int     i;
+           TransactionId *subxids;
+
+           subxids = palloc(sizeof(TransactionId) * nsubxids);
+           memcpy(subxids,
+                  XLogRecGetData(record) + SizeOfCommitTsSet,
+                  sizeof(TransactionId) * nsubxids);
+           for (i = 0; i < nsubxids; i++)
+               appendStringInfo(buf, ", %u", subxids[i]);
+           pfree(subxids);
+       }

There's no need to memcpy() here. The subxid array in the WAL record is
aligned.

* This seems to be a completely unrelated change.

--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -1438,7 +1438,7 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline, int line_num)
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("client certificates can only be checked if a root certificate store is available"),
-                        errhint("Make sure the configuration parameter \"ssl_ca_file\" is set."),
+                        errhint("Make sure the configuration parameter \"%s\" is set.", "ssl_ca_file"),
errcontext("line %d of configuration file \"%s\"",
line_num, HbaFileName)));
return false;

* What is the definition of "latest commit", in
pg_last_committed_xact()? It doesn't necessarily line up with the order
of commit WAL records, nor with the order the proc array is updated
(i.e. the order that the effects become visible to other backends). It
seems confusing to add yet another notion of commit order. Perhaps
that's the best we can do, but it needs to be documented.

- Heikki

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

On 04/12/14 10:42, Heikki Linnakangas wrote:

On 12/03/2014 04:54 PM, Alvaro Herrera wrote:

ir commit timestamp directly as they commit,
or an external transaction c

Sorry, I'm late to the party, but here's some random comments on this
after a quick review:

* The whole concept of a node ID seems to be undocumented, and unused.
No-one calls CommitTsSetDefaultNodeId(). The whole SET_TS record type is
dead code too, when a non-default node_id is not set. Please remove, or
explain how all that's supposed to work.

That's API meant to be used by extensions, maybe it would be preferable
if there was SQL API exposed also?

(It might also make more sense once replication identifiers are submitted.)

* What is the definition of "latest commit", in
pg_last_committed_xact()? It doesn't necessarily line up with the order
of commit WAL records, nor with the order the proc array is updated
(i.e. the order that the effects become visible to other backends). It
seems confusing to add yet another notion of commit order. Perhaps
that's the best we can do, but it needs to be documented.

It's updated on CommitTransaction (well RecordTransactionCommit but
that's only called from CommitTransaction) time so the definition of
latest commit is last CommitTransaction call.

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

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

On 12/04/2014 01:16 PM, Petr Jelinek wrote:

On 04/12/14 10:42, Heikki Linnakangas wrote:

On 12/03/2014 04:54 PM, Alvaro Herrera wrote:

ir commit timestamp directly as they commit,
or an external transaction c

Sorry, I'm late to the party, but here's some random comments on this
after a quick review:

* The whole concept of a node ID seems to be undocumented, and unused.
No-one calls CommitTsSetDefaultNodeId(). The whole SET_TS record type is
dead code too, when a non-default node_id is not set. Please remove, or
explain how all that's supposed to work.

That's API meant to be used by extensions, maybe it would be preferable
if there was SQL API exposed also?

(It might also make more sense once replication identifiers are submitted.)

Maybe. Hard to tell without any documentation or comments on how it's
supposed to work. In unacceptable in its current state.

I would prefer to remove it now, and add it back later together with the
code that actually uses it. I don't like having unused internal
functions and APIs sitting the source tree in the hope that someone will
find them useful in the future. It's more likely that it will bitrot, or
not actually work as intended, when someone later tries to use it.

What would the SQL API look like, and what would it be used for?

* What is the definition of "latest commit", in
pg_last_committed_xact()? It doesn't necessarily line up with the order
of commit WAL records, nor with the order the proc array is updated
(i.e. the order that the effects become visible to other backends). It
seems confusing to add yet another notion of commit order. Perhaps
that's the best we can do, but it needs to be documented.

It's updated on CommitTransaction (well RecordTransactionCommit but
that's only called from CommitTransaction) time so the definition of
latest commit is last CommitTransaction call.

Right. It was a rhetorical question, actually. What I meant is that that
needs to be documented, at least. Or changed so that it matches one of
the other definitions of commit order, and then documented.

- Heikki

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

On 04/12/14 12:26, Heikki Linnakangas wrote:

On 12/04/2014 01:16 PM, Petr Jelinek wrote:

On 04/12/14 10:42, Heikki Linnakangas wrote:

On 12/03/2014 04:54 PM, Alvaro Herrera wrote:

ir commit timestamp directly as they commit,
or an external transaction c

Sorry, I'm late to the party, but here's some random comments on this
after a quick review:

* The whole concept of a node ID seems to be undocumented, and unused.
No-one calls CommitTsSetDefaultNodeId(). The whole SET_TS record type is
dead code too, when a non-default node_id is not set. Please remove, or
explain how all that's supposed to work.

That's API meant to be used by extensions, maybe it would be preferable
if there was SQL API exposed also?

(It might also make more sense once replication identifiers are
submitted.)

Maybe. Hard to tell without any documentation or comments on how it's
supposed to work. In unacceptable in its current state.

I would prefer to remove it now, and add it back later together with the
code that actually uses it. I don't like having unused internal
functions and APIs sitting the source tree in the hope that someone will
find them useful in the future. It's more likely that it will bitrot, or
not actually work as intended, when someone later tries to use it.

The thing is I already have extension for 9.4 where I would use this API
for conflict detection if it was available so it's not about hoping for
somebody to find it useful. There was discussion about this during the
review process because the objection was raised already then.

What would the SQL API look like, and what would it be used for?

It would probably mirror the C one, from my POV it's not needed but
maybe some replication solution would prefer to use this without having
to write C components...

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

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

On 12/04/2014 01:47 PM, Petr Jelinek wrote:

On 04/12/14 12:26, Heikki Linnakangas wrote:

On 12/04/2014 01:16 PM, Petr Jelinek wrote:

On 04/12/14 10:42, Heikki Linnakangas wrote:

On 12/03/2014 04:54 PM, Alvaro Herrera wrote:

ir commit timestamp directly as they commit,
or an external transaction c

Sorry, I'm late to the party, but here's some random comments on this
after a quick review:

* The whole concept of a node ID seems to be undocumented, and unused.
No-one calls CommitTsSetDefaultNodeId(). The whole SET_TS record type is
dead code too, when a non-default node_id is not set. Please remove, or
explain how all that's supposed to work.

That's API meant to be used by extensions, maybe it would be preferable
if there was SQL API exposed also?

(It might also make more sense once replication identifiers are
submitted.)

Maybe. Hard to tell without any documentation or comments on how it's
supposed to work. In unacceptable in its current state.

I would prefer to remove it now, and add it back later together with the
code that actually uses it. I don't like having unused internal
functions and APIs sitting the source tree in the hope that someone will
find them useful in the future. It's more likely that it will bitrot, or
not actually work as intended, when someone later tries to use it.

The thing is I already have extension for 9.4 where I would use this API
for conflict detection if it was available so it's not about hoping for
somebody to find it useful. There was discussion about this during the
review process because the objection was raised already then.

Yeah, it was raised. I don't think it was really addressed. There was
lengthy discussion on whether to include LSN, node id, and/or some other
information, but there was no discussion on:

* What is a node ID?
* How is it used?
* Who assigns it?

etc.

None of those things are explained in the documentation nor code comments.

The node ID sounds suspiciously like the replication identifiers that
have been proposed a couple of times. I don't remember if I like
replication identifiers or not, but I'd rather discuss that issue
explicitly and separately. I don't want the concept of a
replication/node ID to fly under the radar like this.

What would the SQL API look like, and what would it be used for?

It would probably mirror the C one, from my POV it's not needed but
maybe some replication solution would prefer to use this without having
to write C components...

I can't comment on that, because without any documentation, I don't even
know what the C API is.

BTW, why is it OK that the node-ID of a commit is logged as a separate
WAL record, after the commit record? That means that it's possible that
a transaction commits, but you crash just before writing the SETTS
record, so that after replay the transaction appears committed but with
the default node ID.

(Maybe that's OK given the intended use case, but it's hard to tell
without any documentation. See a pattern here? ;-) )

- Heikki

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

Heikki Linnakangas <hlinnakangas@vmware.com> writes:

On 12/03/2014 04:54 PM, Alvaro Herrera wrote:

ir commit timestamp directly as they commit,
or an external transaction c

Sorry, I'm late to the party, but here's some random comments on this
after a quick review:

Also this commit breaks initdb of `make check' for me:

creating template1 database in /home/ash/build/postgresql/HEAD/src/test/regress/./tmp_check/data/base/1 ... TRAP: FailedAssertion("!(((xmax) >= ((TransactionId) 3)))", File: "/home/ash/src/postgresql/src/backend/storage/ipc/procarray.c", Line: 1414)
Aborted (core dumped)
child process exited with exit code 134

--
Alex

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

Alex Shulgin wrote:

Also this commit breaks initdb of `make check' for me:

creating template1 database in /home/ash/build/postgresql/HEAD/src/test/regress/./tmp_check/data/base/1 ... TRAP: FailedAssertion("!(((xmax) >= ((TransactionId) 3)))", File: "/home/ash/src/postgresql/src/backend/storage/ipc/procarray.c", Line: 1414)
Aborted (core dumped)
child process exited with exit code 134

Uh, that's odd. Can you please get a stack trace? Do you have unusual
settings or a patched build?

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

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Alex Shulgin wrote:

Also this commit breaks initdb of `make check' for me:

creating template1 database in
/home/ash/build/postgresql/HEAD/src/test/regress/./tmp_check/data/base/1
... TRAP: FailedAssertion("!(((xmax) >= ((TransactionId) 3)))",
File:
"/home/ash/src/postgresql/src/backend/storage/ipc/procarray.c",
Line: 1414)
Aborted (core dumped)
child process exited with exit code 134

Uh, that's odd. Can you please get a stack trace? Do you have unusual
settings or a patched build?

Not really, and I would mention that. Will get a trace.

--
Alex

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Uh, that's odd. Can you please get a stack trace? Do you have unusual
settings or a patched build?

Is there a way to pause the bootstrap process so I can attach gdb to it?

--
Alex

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

On 12/04/2014 10:50 PM, Alex Shulgin wrote:

Is there a way to pause the bootstrap process so I can attach gdb to it?

With a newer gdb, you can instead tell gdb to follow all forks. I wrote
some notes on it recently.

http://blog.2ndquadrant.com/processes-breakpoints-watchpoints-postgresql/

I've found it really handy when debugging crashes in regression tests.

However, it's often simpler to just:

ulimit -c unlimited
make check

(or whatever your crashing test is) then look at the core files that are
produced.

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

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

Craig Ringer <craig@2ndquadrant.com> writes:

On 12/04/2014 10:50 PM, Alex Shulgin wrote:

Is there a way to pause the bootstrap process so I can attach gdb to it?

With a newer gdb, you can instead tell gdb to follow all forks. I wrote
some notes on it recently.

http://blog.2ndquadrant.com/processes-breakpoints-watchpoints-postgresql/

I've found it really handy when debugging crashes in regression tests.

However, it's often simpler to just:

ulimit -c unlimited
make check

(or whatever your crashing test is) then look at the core files that are
produced.

Good one, it didn't occur to me that assert makes core files.

Figured it out with a pg_usleep in bootstrap.c anyway. Does this look sane?

DEBUG: inserting column 6 value "0"
DEBUG: inserted -> 0
DEBUG: inserting column 7 value "varchar_transform"
TRAP: FailedAssertion("!(((xmax) >= ((TransactionId) 3)))", File: "/home/ash/src/postgresql/src/backend/storage/ipc/procarray.c", Line: 1414)

Program received signal SIGABRT, Aborted.
0x00007f2757128d27 in __GI_raise (sig=sig@entry=6)
at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
56 ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0 0x00007f2757128d27 in __GI_raise (sig=sig@entry=6)
at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1 0x00007f275712a418 in __GI_abort () at abort.c:89
#2 0x00000000009088b2 in ExceptionalCondition (
conditionName=0xac0710 "!(((xmax) >= ((TransactionId) 3)))",
errorType=0xac01d8 "FailedAssertion",
fileName=0xac0178 "/home/ash/src/postgresql/src/backend/storage/ipc/procarray.c", lineNumber=1414)
at /home/ash/src/postgresql/src/backend/utils/error/assert.c:54
#3 0x000000000079e125 in GetSnapshotData (
snapshot=0xdb2d60 <CatalogSnapshotData>)
at /home/ash/src/postgresql/src/backend/storage/ipc/procarray.c:1414
#4 0x000000000094e02d in GetNonHistoricCatalogSnapshot (relid=1255)
at /home/ash/src/postgresql/src/backend/utils/time/snapmgr.c:298
#5 0x000000000094dfdd in GetCatalogSnapshot (relid=1255)
at /home/ash/src/postgresql/src/backend/utils/time/snapmgr.c:272
#6 0x00000000004c8f0d in systable_beginscan (heapRelation=0x1d0e5c0,
indexId=2691, indexOK=1 '\001', snapshot=0x0, nkeys=1, key=0x7fff201bbc40)
at /home/ash/src/postgresql/src/backend/access/index/genam.c:275
#7 0x0000000000885070 in regprocin (fcinfo=0x7fff201bbce0)
at /home/ash/src/postgresql/src/backend/utils/adt/regproc.c:106
#8 0x0000000000914fe7 in InputFunctionCall (flinfo=0x7fff201bc0c0,
str=0x1d349b8 "varchar_transform", typioparam=24, typmod=-1)
---Type <return> to continue, or q <return> to quit---
at /home/ash/src/postgresql/src/backend/utils/fmgr/fmgr.c:1914
#9 0x000000000091533e in OidInputFunctionCall (functionId=44,
str=0x1d349b8 "varchar_transform", typioparam=24, typmod=-1)
at /home/ash/src/postgresql/src/backend/utils/fmgr/fmgr.c:2045
#10 0x000000000052af91 in InsertOneValue (value=0x1d349b8 "varchar_transform",
i=7) at /home/ash/src/postgresql/src/backend/bootstrap/bootstrap.c:840
#11 0x0000000000527409 in boot_yyparse ()
at /home/ash/src/postgresql/src/backend/bootstrap/bootparse.y:455
#12 0x000000000052a26b in BootstrapModeMain ()
at /home/ash/src/postgresql/src/backend/bootstrap/bootstrap.c:494
#13 0x000000000052a177 in AuxiliaryProcessMain (argc=6, argv=0x1cc8378)
at /home/ash/src/postgresql/src/backend/bootstrap/bootstrap.c:414
#14 0x00000000006a327c in main (argc=7, argv=0x1cc8370)
at /home/ash/src/postgresql/src/backend/main/main.c:212
(gdb)

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

Alex Shulgin <ash@commandprompt.com> writes:

Figured it out with a pg_usleep in bootstrap.c anyway. Does this look sane?

DEBUG: inserting column 6 value "0"
DEBUG: inserted -> 0
DEBUG: inserting column 7 value "varchar_transform"
TRAP: FailedAssertion("!(((xmax) >= ((TransactionId) 3)))", File: "/home/ash/src/postgresql/src/backend/storage/ipc/procarray.c", Line: 1414)

I've tried to debug this and I feel really dumbfound...

DEBUG: inserting column 7 value "varchar_transform"

Breakpoint 1, GetSnapshotData (snapshot=0xdb2d60 <CatalogSnapshotData>)
at /home/ash/src/postgresql/src/backend/storage/ipc/procarray.c:1413
1413 xmax = ShmemVariableCache->latestCompletedXid;

(gdb) p ShmemVariableCache->latestCompletedXid
$1 = 4294967295

(gdb) p *ShmemVariableCache
$2 = {nextOid = 10000, oidCount = 0, nextXid = 3, oldestXid = 3,
xidVacLimit = 200000003, xidWarnLimit = 2136483650,
xidStopLimit = 2146483650, xidWrapLimit = 2147483650, oldestXidDB = 1,
oldestCommitTs = 1, newestCommitTs = 0, latestCompletedXid = 4294967295}

(gdb) p xmax
$3 = 0

(gdb) n
1414 Assert(TransactionIdIsNormal(xmax));

(gdb) p xmax
$4 = 1

(gdb) p *ShmemVariableCache
$5 = {nextOid = 10000, oidCount = 0, nextXid = 3, oldestXid = 3,
xidVacLimit = 200000003, xidWarnLimit = 2136483650,
xidStopLimit = 2146483650, xidWrapLimit = 2147483650, oldestXidDB = 1,
oldestCommitTs = 1, newestCommitTs = 0, latestCompletedXid = 4294967295}

(gdb) p ShmemVariableCache->latestCompletedXid
$6 = 4294967295

(gdb)

How? Is there an another concurrent process with the old view of
VariableCacheData struct where latestCompletedXid still points to
oldestCommitTs?

This only happens with the CommitTs commit in effect.

--
Alex

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

Alex Shulgin wrote:

DEBUG: inserting column 7 value "varchar_transform"

Breakpoint 1, GetSnapshotData (snapshot=0xdb2d60 <CatalogSnapshotData>)
at /home/ash/src/postgresql/src/backend/storage/ipc/procarray.c:1413
1413 xmax = ShmemVariableCache->latestCompletedXid;

(gdb) p ShmemVariableCache->latestCompletedXid
$1 = 4294967295

I think you might need to "make distclean", then recompile. If you're
not passing --enable-depend to configure, I suggest you do so; that
greatly reduces such problems.

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

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Alex Shulgin wrote:

DEBUG: inserting column 7 value "varchar_transform"

Breakpoint 1, GetSnapshotData (snapshot=0xdb2d60 <CatalogSnapshotData>)
at /home/ash/src/postgresql/src/backend/storage/ipc/procarray.c:1413
1413 xmax = ShmemVariableCache->latestCompletedXid;

(gdb) p ShmemVariableCache->latestCompletedXid
$1 = 4294967295

I think you might need to "make distclean", then recompile. If you're
not passing --enable-depend to configure, I suggest you do so; that
greatly reduces such problems.

Well I tried that before posting the complaint, let me try again though.

--
Alex

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Alex Shulgin wrote:

DEBUG: inserting column 7 value "varchar_transform"

Breakpoint 1, GetSnapshotData (snapshot=0xdb2d60 <CatalogSnapshotData>)
at /home/ash/src/postgresql/src/backend/storage/ipc/procarray.c:1413
1413 xmax = ShmemVariableCache->latestCompletedXid;

(gdb) p ShmemVariableCache->latestCompletedXid
$1 = 4294967295

I think you might need to "make distclean", then recompile. If you're
not passing --enable-depend to configure, I suggest you do so; that
greatly reduces such problems.

Yeah, that did it. Sorry for the noise, I must have messed it up with
the recompilations (note to self to always use --enable-depend).

--
Alex

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

On Thu, Dec 4, 2014 at 9:26 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:

On 12/04/2014 01:47 PM, Petr Jelinek wrote:

On 04/12/14 12:26, Heikki Linnakangas wrote:

On 12/04/2014 01:16 PM, Petr Jelinek wrote:

On 04/12/14 10:42, Heikki Linnakangas wrote:

On 12/03/2014 04:54 PM, Alvaro Herrera wrote:

ir commit timestamp directly as they commit,
or an external transaction c

Sorry, I'm late to the party, but here's some random comments on this
after a quick review:

* The whole concept of a node ID seems to be undocumented, and unused.
No-one calls CommitTsSetDefaultNodeId(). The whole SET_TS record type
is
dead code too, when a non-default node_id is not set. Please remove, or
explain how all that's supposed to work.

That's API meant to be used by extensions, maybe it would be preferable
if there was SQL API exposed also?

(It might also make more sense once replication identifiers are
submitted.)

Maybe. Hard to tell without any documentation or comments on how it's
supposed to work. In unacceptable in its current state.

I would prefer to remove it now, and add it back later together with the
code that actually uses it. I don't like having unused internal
functions and APIs sitting the source tree in the hope that someone will
find them useful in the future. It's more likely that it will bitrot, or
not actually work as intended, when someone later tries to use it.

The thing is I already have extension for 9.4 where I would use this API
for conflict detection if it was available so it's not about hoping for
somebody to find it useful. There was discussion about this during the
review process because the objection was raised already then.

Yeah, it was raised. I don't think it was really addressed. There was
lengthy discussion on whether to include LSN, node id, and/or some other
information, but there was no discussion on:

* What is a node ID?
* How is it used?
* Who assigns it?

etc.

None of those things are explained in the documentation nor code comments.

The node ID sounds suspiciously like the replication identifiers that have
been proposed a couple of times. I don't remember if I like replication
identifiers or not, but I'd rather discuss that issue explicitly and
separately. I don't want the concept of a replication/node ID to fly under
the radar like this.

What would the SQL API look like, and what would it be used for?

It would probably mirror the C one, from my POV it's not needed but
maybe some replication solution would prefer to use this without having
to write C components...

I can't comment on that, because without any documentation, I don't even
know what the C API is.

BTW, why is it OK that the node-ID of a commit is logged as a separate WAL
record, after the commit record? That means that it's possible that a
transaction commits, but you crash just before writing the SETTS record, so
that after replay the transaction appears committed but with the default
node ID.

(Maybe that's OK given the intended use case, but it's hard to tell without
any documentation. See a pattern here? ;-) )

Could it be possible to get a refresh on that before it bitrots too
much or we'll simply forget about it? In the current state, there is
no documentation able to explain what is the point of the nodeid
stuff, how to use it and what use cases it is aimed at. The API in
committs.c should as well be part of it. If that's not done, I think
that it would be fair to remove this portion and simply WAL-log the
commit timestamp its GUC is activated.
Thanks,
--
Michael

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

On 10/12/14 04:26, Michael Paquier wrote:

On Thu, Dec 4, 2014 at 9:26 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:

Yeah, it was raised. I don't think it was really addressed. There was
lengthy discussion on whether to include LSN, node id, and/or some other
information, but there was no discussion on:

* What is a node ID?
* How is it used?
* Who assigns it?

etc.

None of those things are explained in the documentation nor code comments.

The node ID sounds suspiciously like the replication identifiers that have
been proposed a couple of times. I don't remember if I like replication
identifiers or not, but I'd rather discuss that issue explicitly and
separately. I don't want the concept of a replication/node ID to fly under
the radar like this.

Replication identifiers are bigger thing but yes there is overlap as the
nodeid itself makes it possible to implement replication identifiers
outside of core if needed.

What would the SQL API look like, and what would it be used for?

It would probably mirror the C one, from my POV it's not needed but
maybe some replication solution would prefer to use this without having
to write C components...

I can't comment on that, because without any documentation, I don't even
know what the C API is.

BTW, why is it OK that the node-ID of a commit is logged as a separate WAL
record, after the commit record? That means that it's possible that a
transaction commits, but you crash just before writing the SETTS record, so
that after replay the transaction appears committed but with the default
node ID.

(Maybe that's OK given the intended use case, but it's hard to tell without
any documentation. See a pattern here? ;-) )

This is actually good point, it's not OK to have just commit record but
no nodeid record if nodeid was set.

Could it be possible to get a refresh on that before it bitrots too
much or we'll simply forget about it? In the current state, there is
no documentation able to explain what is the point of the nodeid
stuff, how to use it and what use cases it is aimed at. The API in
committs.c should as well be part of it. If that's not done, I think
that it would be fair to remove this portion and simply WAL-log the
commit timestamp its GUC is activated.

I have this on my list so it's not being forgotten and I don't see it
bitrotting unless we are changing XLog api again. I have bit busy
schedule right now though, can it wait till next week? - I will post
some code documentation patches then.

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

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

On 10/12/14 16:03, Petr Jelinek wrote:

On 10/12/14 04:26, Michael Paquier wrote:

On Thu, Dec 4, 2014 at 9:26 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:

Yeah, it was raised. I don't think it was really addressed. There was
lengthy discussion on whether to include LSN, node id, and/or some other
information, but there was no discussion on:

* What is a node ID?
* How is it used?
* Who assigns it?

etc.

None of those things are explained in the documentation nor code
comments.

Could it be possible to get a refresh on that before it bitrots too
much or we'll simply forget about it? In the current state, there is
no documentation able to explain what is the point of the nodeid
stuff, how to use it and what use cases it is aimed at. The API in
committs.c should as well be part of it. If that's not done, I think
that it would be fair to remove this portion and simply WAL-log the
commit timestamp its GUC is activated.

I have this on my list so it's not being forgotten and I don't see it
bitrotting unless we are changing XLog api again. I have bit busy
schedule right now though, can it wait till next week? - I will post
some code documentation patches then.

Hi,

as promised I am sending code-comment patch that explains the use of
commit timestamps + nodeid C api for the conflict resolution, comments
welcome.

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

On Fri, Dec 19, 2014 at 6:30 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:

On 10/12/14 16:03, Petr Jelinek wrote:

On 10/12/14 04:26, Michael Paquier wrote:

On Thu, Dec 4, 2014 at 9:26 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:

Yeah, it was raised. I don't think it was really addressed. There was
lengthy discussion on whether to include LSN, node id, and/or some other
information, but there was no discussion on:

* What is a node ID?
* How is it used?
* Who assigns it?

etc.

None of those things are explained in the documentation nor code
comments.

Could it be possible to get a refresh on that before it bitrots too
much or we'll simply forget about it? In the current state, there is
no documentation able to explain what is the point of the nodeid
stuff, how to use it and what use cases it is aimed at. The API in
committs.c should as well be part of it. If that's not done, I think
that it would be fair to remove this portion and simply WAL-log the
commit timestamp its GUC is activated.

I have this on my list so it's not being forgotten and I don't see it
bitrotting unless we are changing XLog api again. I have bit busy
schedule right now though, can it wait till next week? - I will post
some code documentation patches then.

as promised I am sending code-comment patch that explains the use of commit
timestamps + nodeid C api for the conflict resolution, comments welcome.

Having some documentation with this example in doc/ would be more fruitful IMO.
--
Michael

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

On 19/12/14 13:17, Michael Paquier wrote:

On Fri, Dec 19, 2014 at 6:30 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:

On 10/12/14 16:03, Petr Jelinek wrote:

On 10/12/14 04:26, Michael Paquier wrote:

On Thu, Dec 4, 2014 at 9:26 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:

Yeah, it was raised. I don't think it was really addressed. There was
lengthy discussion on whether to include LSN, node id, and/or some other
information, but there was no discussion on:

* What is a node ID?
* How is it used?
* Who assigns it?

etc.

None of those things are explained in the documentation nor code
comments.

Could it be possible to get a refresh on that before it bitrots too
much or we'll simply forget about it? In the current state, there is
no documentation able to explain what is the point of the nodeid
stuff, how to use it and what use cases it is aimed at. The API in
committs.c should as well be part of it. If that's not done, I think
that it would be fair to remove this portion and simply WAL-log the
commit timestamp its GUC is activated.

I have this on my list so it's not being forgotten and I don't see it
bitrotting unless we are changing XLog api again. I have bit busy
schedule right now though, can it wait till next week? - I will post
some code documentation patches then.

as promised I am sending code-comment patch that explains the use of commit
timestamps + nodeid C api for the conflict resolution, comments welcome.

Having some documentation with this example in doc/ would be more fruitful IMO.

I am not sure I see point in that, the GUC and SQL interfaces are
documented in doc/ and we usually don't put documentation for C
interfaces there.

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

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

On 12/19/2014 11:30 AM, Petr Jelinek wrote:

as promised I am sending code-comment patch that explains the use of
commit timestamps + nodeid C api for the conflict resolution, comments
welcome.

That's a little bit better, but I have to say I'm still not impressed.
There are so many implicit assumptions in the system. The first
assumption is that a 32-bit node id is sufficient. I'm sure it is for
many replication systems, but might not be for all. Then there's the
assumption that the node id should be "sticky", i.e. it's set for the
whole session. Again, I'm sure that's useful for many systems, but I
could just as easily imagine that you'd want it to reset after every commit.

To be honest, I think this patch should be reverted. Instead, we should
design a system where extensions can define their own SLRUs to store
additional per-transaction information. That way, each extension can
have as much space per transaction as needed, and support functions that
make most sense with the information. Commit timestamp tracking would be
one such extension, and for this node ID stuff, you could have another
one (or include it in the replication extension).

- Heikki

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

On 2014-12-29 12:06:07 +0200, Heikki Linnakangas wrote:

That's a little bit better, but I have to say I'm still not impressed. There
are so many implicit assumptions in the system. The first assumption is that
a 32-bit node id is sufficient.

Seriously? Are we going to build facilities for replication systems with
that many nodes? It seems absolutely unrealistic that a) somebody does
this b) that we'll blindly meet the demands of such a super hypothetical
scenario.

Then there's the assumption that the node id should be "sticky",
i.e. it's set for the whole session. Again, I'm sure that's useful for
many systems, but I could just as easily imagine that you'd want it to
reset after every commit.

It's trivial to add that/reset it manually. So what?

To be honest, I think this patch should be reverted. Instead, we should
design a system where extensions can define their own SLRUs to store
additional per-transaction information. That way, each extension can have as
much space per transaction as needed, and support functions that make most
sense with the information. Commit timestamp tracking would be one such
extension, and for this node ID stuff, you could have another one (or
include it in the replication extension).

If somebody wants that they should develop it. But given that we, based
on previous discussions, don't want to run user defined code in the
relevant phase during transaction commit *and* replay I don't think it'd
be all that easy to do it fast and flexible.

Greetings,

Andres Freund

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

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

On 29/12/14 11:16, Andres Freund wrote:

On 2014-12-29 12:06:07 +0200, Heikki Linnakangas wrote:

That's a little bit better, but I have to say I'm still not impressed. There
are so many implicit assumptions in the system. The first assumption is that
a 32-bit node id is sufficient.

Seriously? Are we going to build facilities for replication systems with
that many nodes? It seems absolutely unrealistic that a) somebody does
this b) that we'll blindly meet the demands of such a super hypothetical
scenario.

+1, Honestly, if somebody will ever have setup with more nodes than what
fits into 32bits they will run into bigger problems than nodeid being
too small.

Then there's the assumption that the node id should be "sticky",
i.e. it's set for the whole session. Again, I'm sure that's useful for
many systems, but I could just as easily imagine that you'd want it to
reset after every commit.

It's trivial to add that/reset it manually. So what?

Yes you can reset in the extension after commit, or you can actually
override both commit timestamp and nodeid after commit if you so wish.

To be honest, I think this patch should be reverted. Instead, we should
design a system where extensions can define their own SLRUs to store
additional per-transaction information. That way, each extension can have as
much space per transaction as needed, and support functions that make most
sense with the information. Commit timestamp tracking would be one such
extension, and for this node ID stuff, you could have another one (or
include it in the replication extension).

If somebody wants that they should develop it. But given that we, based
on previous discussions, don't want to run user defined code in the
relevant phase during transaction commit *and* replay I don't think it'd
be all that easy to do it fast and flexible.

Right, I would love to have custom SLRUs but I don't see it happening
given those two restrictions, otherwise I would write the CommitTs patch
that way in the first place...

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

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

On 12/29/2014 12:39 PM, Petr Jelinek wrote:

On 29/12/14 11:16, Andres Freund wrote:

On 2014-12-29 12:06:07 +0200, Heikki Linnakangas wrote:

To be honest, I think this patch should be reverted. Instead, we should
design a system where extensions can define their own SLRUs to store
additional per-transaction information. That way, each extension can have as
much space per transaction as needed, and support functions that make most
sense with the information. Commit timestamp tracking would be one such
extension, and for this node ID stuff, you could have another one (or
include it in the replication extension).

If somebody wants that they should develop it. But given that we, based
on previous discussions, don't want to run user defined code in the
relevant phase during transaction commit *and* replay I don't think it'd
be all that easy to do it fast and flexible.

Right, I would love to have custom SLRUs but I don't see it happening
given those two restrictions, otherwise I would write the CommitTs patch
that way in the first place...

Transaction commit and replay can treat the per-transaction information
as an opaque blob. It just needs to be included in the commit record,
and replay needs to write it to the SLRU. That way you don't need to run
any user-defined code in those phases.

- Heikki

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

On 2014-12-29 12:50:23 +0200, Heikki Linnakangas wrote:

On 12/29/2014 12:39 PM, Petr Jelinek wrote:

On 29/12/14 11:16, Andres Freund wrote:

On 2014-12-29 12:06:07 +0200, Heikki Linnakangas wrote:

To be honest, I think this patch should be reverted. Instead, we should
design a system where extensions can define their own SLRUs to store
additional per-transaction information. That way, each extension can have as
much space per transaction as needed, and support functions that make most
sense with the information. Commit timestamp tracking would be one such
extension, and for this node ID stuff, you could have another one (or
include it in the replication extension).

If somebody wants that they should develop it. But given that we, based
on previous discussions, don't want to run user defined code in the
relevant phase during transaction commit *and* replay I don't think it'd
be all that easy to do it fast and flexible.

Right, I would love to have custom SLRUs but I don't see it happening
given those two restrictions, otherwise I would write the CommitTs patch
that way in the first place...

Transaction commit and replay can treat the per-transaction information as
an opaque blob. It just needs to be included in the commit record, and
replay needs to write it to the SLRU. That way you don't need to run any
user-defined code in those phases.

Meh. Only if you want to duplicate the timestamps from the commits.

Greetings,

Andres Freund

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

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