Re: [HACKERS] oldest/newestCommitTs output by pg_controldata

2015-12-28 Thread Joe Conway
On 12/28/2015 09:53 AM, Alvaro Herrera wrote:
>>> The name is just as misleading at the source-code level, maybe more so
>>> since they're all just numbers in C.  +1 for changing it everywhere
>>> before somebody makes a mistake based on the incorrect names.
>>
>> Ok, I'm on it now
> 
> Great, thanks.

I think the attached does the job. Note I settled on
(new|old)estCommitTsXid as that seemed most descriptive and not horribly
longer than before. It did mean, however, that I needed to add a single
space to all the output in pg_resetxlog and pg_controldata, which added
a fair amount to the patch size.

It is all fairly straightforward, but given the impending 9.5 release,
it'd be nice if someone has a chance to give this a quick review before
I commit.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
index 83cc9e8..5e210b9 100644
*** a/src/backend/access/rmgrdesc/xlogdesc.c
--- b/src/backend/access/rmgrdesc/xlogdesc.c
*** xlog_desc(StringInfo buf, XLogReaderStat
*** 59,66 
  		 checkpoint->oldestXidDB,
  		 checkpoint->oldestMulti,
  		 checkpoint->oldestMultiDB,
! 		 checkpoint->oldestCommitTs,
! 		 checkpoint->newestCommitTs,
  		 checkpoint->oldestActiveXid,
   (info == XLOG_CHECKPOINT_SHUTDOWN) ? "shutdown" : "online");
  	}
--- 59,66 
  		 checkpoint->oldestXidDB,
  		 checkpoint->oldestMulti,
  		 checkpoint->oldestMultiDB,
! 		 checkpoint->oldestCommitTsXid,
! 		 checkpoint->newestCommitTsXid,
  		 checkpoint->oldestActiveXid,
   (info == XLOG_CHECKPOINT_SHUTDOWN) ? "shutdown" : "online");
  	}
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 8f09dc8..9b106db 100644
*** a/src/backend/access/transam/commit_ts.c
--- b/src/backend/access/transam/commit_ts.c
*** TransactionTreeSetCommitTsData(Transacti
*** 217,224 
  	commitTsShared->dataLastCommit.nodeid = nodeid;
  
  	/* and move forwards our endpoint, if needed */
! 	if (TransactionIdPrecedes(ShmemVariableCache->newestCommitTs, newestXact))
! 		ShmemVariableCache->newestCommitTs = newestXact;
  	LWLockRelease(CommitTsLock);
  }
  
--- 217,224 
  	commitTsShared->dataLastCommit.nodeid = nodeid;
  
  	/* and move forwards our endpoint, if needed */
! 	if (TransactionIdPrecedes(ShmemVariableCache->newestCommitTsXid, newestXact))
! 		ShmemVariableCache->newestCommitTsXid = newestXact;
  	LWLockRelease(CommitTsLock);
  }
  
*** TransactionIdGetCommitTsData(Transaction
*** 285,292 
  	int			entryno = TransactionIdToCTsEntry(xid);
  	int			slotno;
  	CommitTimestampEntry entry;
! 	TransactionId oldestCommitTs;
! 	TransactionId newestCommitTs;
  
  	/* error if the given Xid doesn't normally commit */
  	if (!TransactionIdIsNormal(xid))
--- 285,292 
  	int			entryno = TransactionIdToCTsEntry(xid);
  	int			slotno;
  	CommitTimestampEntry entry;
! 	TransactionId oldestCommitTsXid;
! 	TransactionId newestCommitTsXid;
  
  	/* error if the given Xid doesn't normally commit */
  	if (!TransactionIdIsNormal(xid))
*** TransactionIdGetCommitTsData(Transaction
*** 314,331 
  		return *ts != 0;
  	}
  
! 	oldestCommitTs = ShmemVariableCache->oldestCommitTs;
! 	newestCommitTs = ShmemVariableCache->newestCommitTs;
  	/* neither is invalid, or both are */
! 	Assert(TransactionIdIsValid(oldestCommitTs) == TransactionIdIsValid(newestCommitTs));
  	LWLockRelease(CommitTsLock);
  
  	/*
  	 * Return empty if the requested value is outside our valid range.
  	 */
! 	if (!TransactionIdIsValid(oldestCommitTs) ||
! 		TransactionIdPrecedes(xid, oldestCommitTs) ||
! 		TransactionIdPrecedes(newestCommitTs, xid))
  	{
  		*ts = 0;
  		if (nodeid)
--- 314,331 
  		return *ts != 0;
  	}
  
! 	oldestCommitTsXid = ShmemVariableCache->oldestCommitTsXid;
! 	newestCommitTsXid = ShmemVariableCache->newestCommitTsXid;
  	/* neither is invalid, or both are */
! 	Assert(TransactionIdIsValid(oldestCommitTsXid) == TransactionIdIsValid(newestCommitTsXid));
  	LWLockRelease(CommitTsLock);
  
  	/*
  	 * Return empty if the requested value is outside our valid range.
  	 */
! 	if (!TransactionIdIsValid(oldestCommitTsXid) ||
! 		TransactionIdPrecedes(xid, oldestCommitTsXid) ||
! 		TransactionIdPrecedes(newestCommitTsXid, xid))
  	{
  		*ts = 0;
  		if (nodeid)
*** ActivateCommitTs(void)
*** 655,668 
  	 * enabled again?  It doesn't look like it does, because there should be a
  	 * checkpoint that sets the value to InvalidTransactionId at end of
  	 * recovery; and so any chance of injecting new transactions without
! 	 * CommitTs values would occur after the oldestCommitTs has been set to
  	 * Invalid temporarily.
  	 */
  	LWLockAcquire(CommitTsLock, LW_EXCLUSIVE);
! 	if (ShmemVariableCache->oldestCommitTs == 

Re: [HACKERS] oldest/newestCommitTs output by pg_controldata

2015-12-28 Thread Tom Lane
Joe Conway  writes:
> I think the attached does the job. Note I settled on
> (new|old)estCommitTsXid as that seemed most descriptive and not horribly
> longer than before. It did mean, however, that I needed to add a single
> space to all the output in pg_resetxlog and pg_controldata, which added
> a fair amount to the patch size.

Hm.  That will break all the translations for those messages, no?
Not sure it's worth it.

regards, tom lane


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


Re: [HACKERS] oldest/newestCommitTs output by pg_controldata

2015-12-28 Thread Alvaro Herrera
Joe Conway wrote:
> On 12/28/2015 09:03 AM, Tom Lane wrote:
> > Joe Conway  writes:
> >> Ok, but now next question -- should we just change the user visible
> >> output to oldestCommitXID/newestCommitXID, or should we change the
> >> variable name everywhere it appears in source as well? Looks like each
> >> one appears about 25-30 times scattered across 9 or 10 files. Since they
> >> are new in 9.5, if we're going to change them, I'd think we ought to do
> >> it now or never.
> > 
> > The name is just as misleading at the source-code level, maybe more so
> > since they're all just numbers in C.  +1 for changing it everywhere
> > before somebody makes a mistake based on the incorrect names.
> 
> Ok, I'm on it now

Great, thanks.



-- 
Álvaro Herrerahttp://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


Re: [HACKERS] oldest/newestCommitTs output by pg_controldata

2015-12-28 Thread Joe Conway
On 12/26/2015 06:32 PM, Michael Paquier wrote:
> On Sun, Dec 27, 2015 at 10:08 AM, Joe Conway  wrote:
>> In looking at the exposing pg_controldata as function patch again, it
>> struck me that the following output seems wrong:
>>
>> --
>> Latest checkpoint's oldestCommitTs:   20257
>> Latest checkpoint's newestCommitTs:   84159
>> --
>>
>> Those numbers are XIDs, not timestamps. Shouldn't we either emit the
>> actual timestamps, or else rename those to oldest/newestCommitXID?
> 
> I recall from the commit_ts thread that Alvaro had some real need to
> make those fields XIDs and not timestamps, so +1 for renaming that as
> suggested.

Ok, but now next question -- should we just change the user visible
output to oldestCommitXID/newestCommitXID, or should we change the
variable name everywhere it appears in source as well? Looks like each
one appears about 25-30 times scattered across 9 or 10 files. Since they
are new in 9.5, if we're going to change them, I'd think we ought to do
it now or never.

If there is consensus to make the change either way (output-only or
globally), I'll come up with a patch ASAP.

Opinions?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature


Re: [HACKERS] oldest/newestCommitTs output by pg_controldata

2015-12-28 Thread Tom Lane
Joe Conway  writes:
> Ok, but now next question -- should we just change the user visible
> output to oldestCommitXID/newestCommitXID, or should we change the
> variable name everywhere it appears in source as well? Looks like each
> one appears about 25-30 times scattered across 9 or 10 files. Since they
> are new in 9.5, if we're going to change them, I'd think we ought to do
> it now or never.

The name is just as misleading at the source-code level, maybe more so
since they're all just numbers in C.  +1 for changing it everywhere
before somebody makes a mistake based on the incorrect names.

regards, tom lane


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


Re: [HACKERS] oldest/newestCommitTs output by pg_controldata

2015-12-28 Thread Joe Conway
On 12/28/2015 09:03 AM, Tom Lane wrote:
> Joe Conway  writes:
>> Ok, but now next question -- should we just change the user visible
>> output to oldestCommitXID/newestCommitXID, or should we change the
>> variable name everywhere it appears in source as well? Looks like each
>> one appears about 25-30 times scattered across 9 or 10 files. Since they
>> are new in 9.5, if we're going to change them, I'd think we ought to do
>> it now or never.
> 
> The name is just as misleading at the source-code level, maybe more so
> since they're all just numbers in C.  +1 for changing it everywhere
> before somebody makes a mistake based on the incorrect names.

Ok, I'm on it now

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature


Re: [HACKERS] oldest/newestCommitTs output by pg_controldata

2015-12-28 Thread Joe Conway
On 12/28/2015 10:35 AM, Joe Conway wrote:
> An alternative would be to not have a space at all for those two, e.g.
> 
>   "Latest checkpoint's oldestCommitTsXid:%u\n"
>   "Latest checkpoint's newestCommitTsXid:%u\n"
> 
> That isn't too bad and would leave everything aligned.

That seems like the best solution to me.

8<---
...
Latest checkpoint's oldestMultiXid:   1
Latest checkpoint's oldestMulti's DB: 1
Latest checkpoint's oldestCommitTsXid:0
Latest checkpoint's newestCommitTsXid:0
Time of latest checkpoint:Thu 24 Dec 2015 08:55:27 AM PST
Fake LSN counter for unlogged rels:   0/1
...
8<---

Anyone parsing based on fixed length would be ok, and so would anyone
splitting on the colon.

I retract my earlier suggestion of doing HEAD different from
REL9_5_STABLE, at least for the moment. My patch for pg_controldata
related functions is going to impact all this anyway, so we might as
well not fuss about it now.

Any objections to the attached?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
index 83cc9e8..5e210b9 100644
*** a/src/backend/access/rmgrdesc/xlogdesc.c
--- b/src/backend/access/rmgrdesc/xlogdesc.c
*** xlog_desc(StringInfo buf, XLogReaderStat
*** 59,66 
  		 checkpoint->oldestXidDB,
  		 checkpoint->oldestMulti,
  		 checkpoint->oldestMultiDB,
! 		 checkpoint->oldestCommitTs,
! 		 checkpoint->newestCommitTs,
  		 checkpoint->oldestActiveXid,
   (info == XLOG_CHECKPOINT_SHUTDOWN) ? "shutdown" : "online");
  	}
--- 59,66 
  		 checkpoint->oldestXidDB,
  		 checkpoint->oldestMulti,
  		 checkpoint->oldestMultiDB,
! 		 checkpoint->oldestCommitTsXid,
! 		 checkpoint->newestCommitTsXid,
  		 checkpoint->oldestActiveXid,
   (info == XLOG_CHECKPOINT_SHUTDOWN) ? "shutdown" : "online");
  	}
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index ed8d98a..a284894 100644
*** a/src/backend/access/transam/commit_ts.c
--- b/src/backend/access/transam/commit_ts.c
*** TransactionTreeSetCommitTsData(Transacti
*** 217,224 
  	commitTsShared->dataLastCommit.nodeid = nodeid;
  
  	/* and move forwards our endpoint, if needed */
! 	if (TransactionIdPrecedes(ShmemVariableCache->newestCommitTs, newestXact))
! 		ShmemVariableCache->newestCommitTs = newestXact;
  	LWLockRelease(CommitTsLock);
  }
  
--- 217,224 
  	commitTsShared->dataLastCommit.nodeid = nodeid;
  
  	/* and move forwards our endpoint, if needed */
! 	if (TransactionIdPrecedes(ShmemVariableCache->newestCommitTsXid, newestXact))
! 		ShmemVariableCache->newestCommitTsXid = newestXact;
  	LWLockRelease(CommitTsLock);
  }
  
*** TransactionIdGetCommitTsData(Transaction
*** 285,292 
  	int			entryno = TransactionIdToCTsEntry(xid);
  	int			slotno;
  	CommitTimestampEntry entry;
! 	TransactionId oldestCommitTs;
! 	TransactionId newestCommitTs;
  
  	/* error if the given Xid doesn't normally commit */
  	if (!TransactionIdIsNormal(xid))
--- 285,292 
  	int			entryno = TransactionIdToCTsEntry(xid);
  	int			slotno;
  	CommitTimestampEntry entry;
! 	TransactionId oldestCommitTsXid;
! 	TransactionId newestCommitTsXid;
  
  	/* error if the given Xid doesn't normally commit */
  	if (!TransactionIdIsNormal(xid))
*** TransactionIdGetCommitTsData(Transaction
*** 314,331 
  		return *ts != 0;
  	}
  
! 	oldestCommitTs = ShmemVariableCache->oldestCommitTs;
! 	newestCommitTs = ShmemVariableCache->newestCommitTs;
  	/* neither is invalid, or both are */
! 	Assert(TransactionIdIsValid(oldestCommitTs) == TransactionIdIsValid(newestCommitTs));
  	LWLockRelease(CommitTsLock);
  
  	/*
  	 * Return empty if the requested value is outside our valid range.
  	 */
! 	if (!TransactionIdIsValid(oldestCommitTs) ||
! 		TransactionIdPrecedes(xid, oldestCommitTs) ||
! 		TransactionIdPrecedes(newestCommitTs, xid))
  	{
  		*ts = 0;
  		if (nodeid)
--- 314,331 
  		return *ts != 0;
  	}
  
! 	oldestCommitTsXid = ShmemVariableCache->oldestCommitTsXid;
! 	newestCommitTsXid = ShmemVariableCache->newestCommitTsXid;
  	/* neither is invalid, or both are */
! 	Assert(TransactionIdIsValid(oldestCommitTsXid) == TransactionIdIsValid(newestCommitTsXid));
  	LWLockRelease(CommitTsLock);
  
  	/*
  	 * Return empty if the requested value is outside our valid range.
  	 */
! 	if (!TransactionIdIsValid(oldestCommitTsXid) ||
! 		TransactionIdPrecedes(xid, oldestCommitTsXid) ||
! 		TransactionIdPrecedes(newestCommitTsXid, xid))
  	{
  		*ts = 0;
  		if (nodeid)
*** ActivateCommitTs(void)
*** 655,668 
  	 * enabled again?  It doesn't look like it does, because there should be a
  	 * checkpoint that sets the value to InvalidTransactionId at end of
  	 * recovery; and so any chance of 

Re: [HACKERS] oldest/newestCommitTs output by pg_controldata

2015-12-28 Thread Tom Lane
Joe Conway  writes:
> I retract my earlier suggestion of doing HEAD different from
> REL9_5_STABLE, at least for the moment. My patch for pg_controldata
> related functions is going to impact all this anyway, so we might as
> well not fuss about it now.

Seems reasonable.

> Any objections to the attached?

Looks OK in a quick once-over.

regards, tom lane


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


Re: [HACKERS] oldest/newestCommitTs output by pg_controldata

2015-12-28 Thread Joe Conway
On 12/28/2015 10:30 AM, Alvaro Herrera wrote:
> Tom Lane wrote:
>> Joe Conway  writes:
>>> I think the attached does the job. Note I settled on
>>> (new|old)estCommitTsXid as that seemed most descriptive and not horribly
>>> longer than before. It did mean, however, that I needed to add a single
>>> space to all the output in pg_resetxlog and pg_controldata, which added
>>> a fair amount to the patch size.
>>
>> Hm.  That will break all the translations for those messages, no?
>> Not sure it's worth it.
> 
> Yeah, I would leave the others alone and just let this line have the
> value shifted one more column to the right.

Seems like the translation issue would be mostly 9.5. Maybe we should
add the space in master but not 9.5?

An alternative would be to not have a space at all for those two, e.g.

  "Latest checkpoint's oldestCommitTsXid:%u\n"
  "Latest checkpoint's newestCommitTsXid:%u\n"

That isn't too bad and would leave everything aligned.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature


Re: [HACKERS] oldest/newestCommitTs output by pg_controldata

2015-12-28 Thread Alvaro Herrera
Tom Lane wrote:
> Joe Conway  writes:
> > I think the attached does the job. Note I settled on
> > (new|old)estCommitTsXid as that seemed most descriptive and not horribly
> > longer than before. It did mean, however, that I needed to add a single
> > space to all the output in pg_resetxlog and pg_controldata, which added
> > a fair amount to the patch size.
> 
> Hm.  That will break all the translations for those messages, no?
> Not sure it's worth it.

Yeah, I would leave the others alone and just let this line have the
value shifted one more column to the right.

-- 
Álvaro Herrerahttp://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


Re: [HACKERS] oldest/newestCommitTs output by pg_controldata

2015-12-28 Thread Joe Conway
On 12/28/2015 11:48 AM, Tom Lane wrote:
> Joe Conway  writes:
>> I retract my earlier suggestion of doing HEAD different from
>> REL9_5_STABLE, at least for the moment. My patch for pg_controldata
>> related functions is going to impact all this anyway, so we might as
>> well not fuss about it now.
> 
> Seems reasonable.
> 
>> Any objections to the attached?
> 
> Looks OK in a quick once-over.

Pushed to HEAD and 9.5

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature


Re: [HACKERS] oldest/newestCommitTs output by pg_controldata

2015-12-26 Thread Michael Paquier
On Sun, Dec 27, 2015 at 10:08 AM, Joe Conway  wrote:
> In looking at the exposing pg_controldata as function patch again, it
> struck me that the following output seems wrong:
>
> --
> Latest checkpoint's oldestCommitTs:   20257
> Latest checkpoint's newestCommitTs:   84159
> --
>
> Those numbers are XIDs, not timestamps. Shouldn't we either emit the
> actual timestamps, or else rename those to oldest/newestCommitXID?

I recall from the commit_ts thread that Alvaro had some real need to
make those fields XIDs and not timestamps, so +1 for renaming that as
suggested.
-- 
Michael


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