Thread: oldest/newestCommitTs output by pg_controldata
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? Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Sun, Dec 27, 2015 at 10:08 AM, Joe Conway <mail@joeconway.com> 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
On 12/26/2015 06:32 PM, Michael Paquier wrote: > On Sun, Dec 27, 2015 at 10:08 AM, Joe Conway <mail@joeconway.com> 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
Joe Conway <mail@joeconway.com> 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
On 12/28/2015 09:03 AM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> 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
Joe Conway wrote: > On 12/28/2015 09:03 AM, Tom Lane wrote: > > Joe Conway <mail@joeconway.com> 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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
Attachment
Joe Conway <mail@joeconway.com> 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
Tom Lane wrote: > Joe Conway <mail@joeconway.com> 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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 12/28/2015 10:30 AM, Alvaro Herrera wrote: > Tom Lane wrote: >> Joe Conway <mail@joeconway.com> 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
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
Attachment
Joe Conway <mail@joeconway.com> 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
On 12/28/2015 11:48 AM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> 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