Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)
On Sat, Feb 13, 2016 at 7:54 AM, Bruce Momjian wrote: > On Thu, Feb 11, 2016 at 07:18:46PM -0500, Bruce Momjian wrote: >> No, that is not an improvement --- see my previous comment: >> >> > We could get more sophisticated by checking the catalog version number >> > where the format was changed, but that doesn't seem worth it, and is >> > overly complex because we get the catalog version number from >> > pg_controldata, so you would be adding a dependency in ordering of the >> > pg_controldata entries. >> >> By testing for '906', you prevent users from using pg_upgrade to go from >> one catalog version of 9.6 to a later one. Few people may want to do >> it, but it should work. > > C comment added explaining why we do this. Thanks for the addition. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)
On Thu, Feb 11, 2016 at 07:18:46PM -0500, Bruce Momjian wrote: > No, that is not an improvement --- see my previous comment: > > > We could get more sophisticated by checking the catalog version number > > where the format was changed, but that doesn't seem worth it, and is > > overly complex because we get the catalog version number from > > pg_controldata, so you would be adding a dependency in ordering of the > > pg_controldata entries. > > By testing for '906', you prevent users from using pg_upgrade to go from > one catalog version of 9.6 to a later one. Few people may want to do > it, but it should work. C comment added explaining why we do this. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)
On 02/11/2016 04:59 PM, Michael Paquier wrote: > On Fri, Feb 12, 2016 at 9:18 AM, Bruce Momjian wrote: >> No, that is not an improvement --- see my previous comment: >> >>> We could get more sophisticated by checking the catalog version number >>> where the format was changed, but that doesn't seem worth it, and is >>> overly complex because we get the catalog version number from >>> pg_controldata, so you would be adding a dependency in ordering of the >>> pg_controldata entries. >> >> By testing for '906', you prevent users from using pg_upgrade to go from >> one catalog version of 9.6 to a later one. Few people may want to do >> it, but it should work. > > OK, I see now. I did not consider the case where people would like to > get upgrade from a dev version of 9.6 to the latest 9.6 version, just > the upgrade from a previous major version <= 9.5. Thanks for reminding > that pg_upgrade needs to support that. Pushed with Bruce's original patch included. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)
On Fri, Feb 12, 2016 at 9:18 AM, Bruce Momjian wrote: > On Wed, Feb 10, 2016 at 10:23:41AM +0900, Michael Paquier wrote: >> On Wed, Feb 10, 2016 at 9:57 AM, Joe Conway wrote: >> > I'll commit the attached tomorrow if there are no other concerns voiced. >> >> Just a nitpick regarding this block: >> + if (strchr(p, '/') != NULL) >> + p = strchr(p, '/'); >> + /* delimiter changed from '/' to ':' in 9.6 */ >> + else if (GET_MAJOR_VERSION(cluster->major_version) >= 906) >> + p = strchr(p, ':'); >> + else >> + p = NULL; >> Changing it as follows would save some instructions because there is >> no need to call strchr an extra time: >> if (GET_MAJOR_VERSION(cluster->major_version) >= 906) >> p = strchr(p, ':'); >> else >> p = strchr(p, '/'); > > No, that is not an improvement --- see my previous comment: > >> We could get more sophisticated by checking the catalog version number >> where the format was changed, but that doesn't seem worth it, and is >> overly complex because we get the catalog version number from >> pg_controldata, so you would be adding a dependency in ordering of the >> pg_controldata entries. > > By testing for '906', you prevent users from using pg_upgrade to go from > one catalog version of 9.6 to a later one. Few people may want to do > it, but it should work. OK, I see now. I did not consider the case where people would like to get upgrade from a dev version of 9.6 to the latest 9.6 version, just the upgrade from a previous major version <= 9.5. Thanks for reminding that pg_upgrade needs to support that. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)
On Wed, Feb 10, 2016 at 10:23:41AM +0900, Michael Paquier wrote: > On Wed, Feb 10, 2016 at 9:57 AM, Joe Conway wrote: > > I'll commit the attached tomorrow if there are no other concerns voiced. > > Just a nitpick regarding this block: > + if (strchr(p, '/') != NULL) > + p = strchr(p, '/'); > + /* delimiter changed from '/' to ':' in 9.6 */ > + else if (GET_MAJOR_VERSION(cluster->major_version) >= 906) > + p = strchr(p, ':'); > + else > + p = NULL; > Changing it as follows would save some instructions because there is > no need to call strchr an extra time: > if (GET_MAJOR_VERSION(cluster->major_version) >= 906) > p = strchr(p, ':'); > else > p = strchr(p, '/'); No, that is not an improvement --- see my previous comment: > We could get more sophisticated by checking the catalog version number > where the format was changed, but that doesn't seem worth it, and is > overly complex because we get the catalog version number from > pg_controldata, so you would be adding a dependency in ordering of the > pg_controldata entries. By testing for '906', you prevent users from using pg_upgrade to go from one catalog version of 9.6 to a later one. Few people may want to do it, but it should work. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)
On Wed, Feb 10, 2016 at 9:57 AM, Joe Conway wrote: > I'll commit the attached tomorrow if there are no other concerns voiced. Just a nitpick regarding this block: + if (strchr(p, '/') != NULL) + p = strchr(p, '/'); + /* delimiter changed from '/' to ':' in 9.6 */ + else if (GET_MAJOR_VERSION(cluster->major_version) >= 906) + p = strchr(p, ':'); + else + p = NULL; Changing it as follows would save some instructions because there is no need to call strchr an extra time: if (GET_MAJOR_VERSION(cluster->major_version) >= 906) p = strchr(p, ':'); else p = strchr(p, '/'); > In the spirit of the dev meeting discussion, I am trying to use the > commit message template discussed. Something like: > > -- email subject limit - > Change delimiter used for display of NextXID > > NextXID has been rendered in the form of a pg_lsn even though it > really is not. This can cause confusion, so change the format from > %u/%u to %u:%u, per discussion on hackers. > > Complaint by me, patch by me and Bruce, reviewed by Michael Paquier > and Alvaro. Applied to HEAD only. > > Reported-by: Joe Conway > Author: Joe Conway, Bruce Momjian > Reviewed-by: Michael Paquier, Alvaro Herrera > Tested-by: Michael Paquier > Backpatch-through: master > That does look pretty redundant though. Thoughts? Removing Tested-by would be fine here I guess, testing and review are overlapping concepts for this patch. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)
On 01/19/2016 07:04 PM, Michael Paquier wrote:
> On Wed, Jan 20, 2016 at 11:41 AM, Alvaro Herrera
> wrote:
>> Joe Conway wrote:
>>
>>> The attached includes Bruce's change, plus I found two additional sites
>>> that appear to need the same change. The xlog.c change is just a DEBUG
>>> message, so not a big deal. I'm less certain if the xlogdesc.c change
>>> might create some fallout.
>>
>> Hm, pg_xlogdump links the rmgrdesc files, so perhaps you might need to
>> adjust expected test output for it. Not really sure.
>
> We don't depend on this output format in any tests AFAIK, at least
> check-world is not complaining here and pg_xlogdump has no dedicated
> tests. There may be some utility in the outside world doing some
> manipulation of the string generated for this record, but that's not
> worth worrying about anyway.
>
> Patch looks fine, I have not spotted any other places that need a refresh.
I'll commit the attached tomorrow if there are no other concerns voiced.
In the spirit of the dev meeting discussion, I am trying to use the
commit message template discussed. Something like:
-- email subject limit -
Change delimiter used for display of NextXID
NextXID has been rendered in the form of a pg_lsn even though it
really is not. This can cause confusion, so change the format from
%u/%u to %u:%u, per discussion on hackers.
Complaint by me, patch by me and Bruce, reviewed by Michael Paquier
and Alvaro. Applied to HEAD only.
Reported-by: Joe Conway
Author: Joe Conway, Bruce Momjian
Reviewed-by: Michael Paquier, Alvaro Herrera
Tested-by: Michael Paquier
Backpatch-through: master
-- email subject limit -
That does look pretty redundant though. Thoughts?
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 b694dea..2dcbfbd 100644
*** a/src/backend/access/rmgrdesc/xlogdesc.c
--- b/src/backend/access/rmgrdesc/xlogdesc.c
*** xlog_desc(StringInfo buf, XLogReaderStat
*** 43,49
CheckPoint *checkpoint = (CheckPoint *) rec;
appendStringInfo(buf, "redo %X/%X; "
! "tli %u; prev tli %u; fpw %s; xid %u/%u; oid %u; multi %u; offset %u; "
"oldest xid %u in DB %u; oldest multi %u in DB %u; "
"oldest/newest commit timestamp xid: %u/%u; "
"oldest running xid %u; %s",
--- 43,49
CheckPoint *checkpoint = (CheckPoint *) rec;
appendStringInfo(buf, "redo %X/%X; "
! "tli %u; prev tli %u; fpw %s; xid %u:%u; oid %u; multi %u; offset %u; "
"oldest xid %u in DB %u; oldest multi %u in DB %u; "
"oldest/newest commit timestamp xid: %u/%u; "
"oldest running xid %u; %s",
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7d5d493..ee87e1b 100644
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
*** StartupXLOG(void)
*** 6284,6290
(uint32) (checkPoint.redo >> 32), (uint32) checkPoint.redo,
wasShutdown ? "TRUE" : "FALSE")));
ereport(DEBUG1,
! (errmsg_internal("next transaction ID: %u/%u; next OID: %u",
checkPoint.nextXidEpoch, checkPoint.nextXid,
checkPoint.nextOid)));
ereport(DEBUG1,
--- 6284,6290
(uint32) (checkPoint.redo >> 32), (uint32) checkPoint.redo,
wasShutdown ? "TRUE" : "FALSE")));
ereport(DEBUG1,
! (errmsg_internal("next transaction ID: %u:%u; next OID: %u",
checkPoint.nextXidEpoch, checkPoint.nextXid,
checkPoint.nextOid)));
ereport(DEBUG1,
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index e7e072f..5dd2dbc 100644
*** a/src/bin/pg_controldata/pg_controldata.c
--- b/src/bin/pg_controldata/pg_controldata.c
*** main(int argc, char *argv[])
*** 252,258
ControlFile.checkPointCopy.PrevTimeLineID);
printf(_("Latest checkpoint's full_page_writes: %s\n"),
ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
! printf(_("Latest checkpoint's NextXID: %u/%u\n"),
ControlFile.checkPointCopy.nextXidEpoch,
ControlFile.checkPointCopy.nextXid);
printf(_("Latest checkpoint's NextOID: %u\n"),
--- 252,258
ControlFile.checkPointCopy.PrevTimeLineID);
printf(_("Latest checkpoint's full_page_writes: %s\n"),
ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
! printf(_("Latest checkpoint's NextXID: %u:%u\n"),
ControlFile.checkPointCopy.nextXidEpoch,
ControlFile.checkPointCopy.nextXid);
printf(_("Latest checkpoint's NextOID: %u\n"),
diff --git a/src/bin/pg_resetxlog/pg_resetxlog.c b/src/bin/pg_resetxlog/pg_resetxlog.c
index ca706a5..525b82b 100644
*** a/src/bin/pg_resetxlog/pg_resetxlog.c
--- b/src/bin/pg_resetxlog/pg_
Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)
On Wed, Jan 20, 2016 at 11:41 AM, Alvaro Herrera wrote: > Joe Conway wrote: > >> The attached includes Bruce's change, plus I found two additional sites >> that appear to need the same change. The xlog.c change is just a DEBUG >> message, so not a big deal. I'm less certain if the xlogdesc.c change >> might create some fallout. > > Hm, pg_xlogdump links the rmgrdesc files, so perhaps you might need to > adjust expected test output for it. Not really sure. We don't depend on this output format in any tests AFAIK, at least check-world is not complaining here and pg_xlogdump has no dedicated tests. There may be some utility in the outside world doing some manipulation of the string generated for this record, but that's not worth worrying about anyway. Patch looks fine, I have not spotted any other places that need a refresh. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)
Joe Conway wrote: > The attached includes Bruce's change, plus I found two additional sites > that appear to need the same change. The xlog.c change is just a DEBUG > message, so not a big deal. I'm less certain if the xlogdesc.c change > might create some fallout. Hm, pg_xlogdump links the rmgrdesc files, so perhaps you might need to adjust expected test output for it. Not really sure. -- Á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
NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)
On 01/19/2016 09:02 AM, Bruce Momjian wrote:
> Ok. Notwithstanding Simon's reply, there seems to be consensus that this
> is the way to go. Will commit it this way unless some additional
> objections surface in the next day or so.
FYI, this slash-colon change will break pg_upgrade unless it is patched.
Dp you want a patch from me?
>>>
>>> Didn't realize that -- yes please.
>>
>> Sure, attached, and it would be applied only to head, where you change
>> pg_controldata. pg_upgrade has to read the old and new cluster's
>> pg_controldata. We could get more sophisticated by checking the catalog
>> version number where the format was changed, but that doesn't seem worth
>> it, and is overly complex because we get the catalog version number from
>> pg_controldata, so you would be adding a dependency in ordering of the
>> pg_controldata entries.
>
> Sorry, please use the attached patch instead, now tested with your
> changes.
The attached includes Bruce's change, plus I found two additional sites
that appear to need the same change. The xlog.c change is just a DEBUG
message, so not a big deal. I'm less certain if the xlogdesc.c change
might create some fallout.
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 b694dea..2dcbfbd 100644
*** a/src/backend/access/rmgrdesc/xlogdesc.c
--- b/src/backend/access/rmgrdesc/xlogdesc.c
*** xlog_desc(StringInfo buf, XLogReaderStat
*** 43,49
CheckPoint *checkpoint = (CheckPoint *) rec;
appendStringInfo(buf, "redo %X/%X; "
! "tli %u; prev tli %u; fpw %s; xid %u/%u; oid %u; multi %u; offset %u; "
"oldest xid %u in DB %u; oldest multi %u in DB %u; "
"oldest/newest commit timestamp xid: %u/%u; "
"oldest running xid %u; %s",
--- 43,49
CheckPoint *checkpoint = (CheckPoint *) rec;
appendStringInfo(buf, "redo %X/%X; "
! "tli %u; prev tli %u; fpw %s; xid %u:%u; oid %u; multi %u; offset %u; "
"oldest xid %u in DB %u; oldest multi %u in DB %u; "
"oldest/newest commit timestamp xid: %u/%u; "
"oldest running xid %u; %s",
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7d5d493..ee87e1b 100644
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
*** StartupXLOG(void)
*** 6284,6290
(uint32) (checkPoint.redo >> 32), (uint32) checkPoint.redo,
wasShutdown ? "TRUE" : "FALSE")));
ereport(DEBUG1,
! (errmsg_internal("next transaction ID: %u/%u; next OID: %u",
checkPoint.nextXidEpoch, checkPoint.nextXid,
checkPoint.nextOid)));
ereport(DEBUG1,
--- 6284,6290
(uint32) (checkPoint.redo >> 32), (uint32) checkPoint.redo,
wasShutdown ? "TRUE" : "FALSE")));
ereport(DEBUG1,
! (errmsg_internal("next transaction ID: %u:%u; next OID: %u",
checkPoint.nextXidEpoch, checkPoint.nextXid,
checkPoint.nextOid)));
ereport(DEBUG1,
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index e7e072f..5dd2dbc 100644
*** a/src/bin/pg_controldata/pg_controldata.c
--- b/src/bin/pg_controldata/pg_controldata.c
*** main(int argc, char *argv[])
*** 252,258
ControlFile.checkPointCopy.PrevTimeLineID);
printf(_("Latest checkpoint's full_page_writes: %s\n"),
ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
! printf(_("Latest checkpoint's NextXID: %u/%u\n"),
ControlFile.checkPointCopy.nextXidEpoch,
ControlFile.checkPointCopy.nextXid);
printf(_("Latest checkpoint's NextOID: %u\n"),
--- 252,258
ControlFile.checkPointCopy.PrevTimeLineID);
printf(_("Latest checkpoint's full_page_writes: %s\n"),
ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
! printf(_("Latest checkpoint's NextXID: %u:%u\n"),
ControlFile.checkPointCopy.nextXidEpoch,
ControlFile.checkPointCopy.nextXid);
printf(_("Latest checkpoint's NextOID: %u\n"),
diff --git a/src/bin/pg_resetxlog/pg_resetxlog.c b/src/bin/pg_resetxlog/pg_resetxlog.c
index ca706a5..525b82b 100644
*** a/src/bin/pg_resetxlog/pg_resetxlog.c
--- b/src/bin/pg_resetxlog/pg_resetxlog.c
*** PrintControlValues(bool guessed)
*** 646,652
ControlFile.checkPointCopy.ThisTimeLineID);
printf(_("Latest checkpoint's full_page_writes: %s\n"),
ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
! printf(_("Latest checkpoint's NextXID: %u/%u\n"),
ControlFile.checkPointCopy.nextXidEpoch,
ControlFile.checkPointCopy.nextXid);
printf(_("Latest checkpoint's NextOID: %u\n"),
--- 646,652
ControlFile.checkPointCopy.ThisTimeLineID);
printf(_("Latest checkpoi
