Thread: [HACKERS] pg_upgrade to clusters with a different WAL segment size
Hi hackers, The thread for the recent change to allow setting the WAL segment size at initdb time [0] included a bit of discussion regarding pg_upgrade [1], where it was suggested that relaxing an error check (presumably in check_control_data()) might be enough to upgrade servers to a different WAL segment size. We've had success with our initial testing of upgrades to larger WAL segment sizes, including post-upgrade pgbench runs. Beyond adjusting check_control_data(), it looks like the 'pg_resetwal -l' call in copy_xact_xlog_xid() may need to be adjusted to ensure that the WAL starting address is set to a valid value. Allowing changes to the WAL segment size during pg_upgrade seems like a nice way to avoid needing a dump and load, so I would like to propose adding support for this. I'd be happy to submit patches for this in the next commitfest. Nathan [0] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=fc49e24fa69a15efacd5b8958115ed9c43c48f9a [1] https://www.postgresql.org/message-id/20160826043926.pwkz45ksxpmfn4g6%40alap3.anarazel.de -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Nov 11, 2017 at 12:46 AM, Bossart, Nathan <bossartn@amazon.com> wrote: > Allowing changes to the WAL segment size during pg_upgrade seems like a > nice way to avoid needing a dump and load, so I would like to propose > adding support for this. I'd be happy to submit patches for this in the > next commitfest. That's a worthy goal. -- 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 Fri, Nov 10, 2017 at 4:04 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Sat, Nov 11, 2017 at 12:46 AM, Bossart, Nathan <bossartn@amazon.com> wrote:
>> Allowing changes to the WAL segment size during pg_upgrade seems like a
>> nice way to avoid needing a dump and load, so I would like to propose
>> adding support for this. I'd be happy to submit patches for this in the
>> next commitfest.
>
> That's a worthy goal.
I'm also interested in this item and I helped Nathan with a little of
the initial testing. Also, having changed redo sizes on other
database platforms a couple times (a simple & safe runtime operation
there), it seems to me that a feature like this would benefit
PostgreSQL.
I would add that we increased the max segment size in pg10 - but the
handful of users who are in the most pain with very high activity
rates on running systems are still limited to logical upgrades or
dump-and-load to get the benefit of larger WAL segment sizes. From a
technical perspective, it doesn't seem like it should be too
complicated to implement this in pg_upgrade since you're moving into a
new cluster anyway.
On Fri, Nov 10, 2017 at 7:46 AM, Bossart, Nathan <bossartn@amazon.com> wrote:
> We've had success with our initial testing of upgrades to larger WAL
> segment sizes, including post-upgrade pgbench runs.
Just to fill this out a little; our very first test was to take a
9.6.5 16mb-wal post-pgbench db and pg_upgrade it to 10.0 128mb-wal
with no changes except removing the WAL size from check_control_data()
then doing more pgbench runs on the same db post-upgrade. Checked for
errors or problematic variation in TPS. More of a smoke-screen than a
thorough test, but everything looks good so far.
On Fri, Nov 10, 2017 at 7:46 AM, Bossart, Nathan <bossartn@amazon.com> wrote:
> Beyond adjusting
> check_control_data(), it looks like the 'pg_resetwal -l' call in
> copy_xact_xlog_xid() may need to be adjusted to ensure that the WAL
> starting address is set to a valid value.
This was related to one interesting quirk we observed. The pg_upgrade
tried to call pg_resetwal on the *new* database with a log sequence
number that assumes the *old* wal size. In our test, it called
"pg_resetwal -l 000000010000000200000071" which is an invalid filename
with 128mb wal segment. In order to get a sensible filename,
PostgreSQL took the "71" and wrapped three times and added to get a
new WAL filename of "000000010000000500000011".
This actually raises a really interesting concern with pg_upgrade and
different WAL segment sizes. We have WAL filenames and then we have
XLogSegNo. If pg_upgrade just chooses the next valid filename, then
XLogSegNo will decrease and overlap when the WAL segment size goes up.
If pg_upgrade calculates the next XLogSegNo then the WAL segment
filename will decrease and overlap when the WAL segment size goes
down.
from xlog_internal.h:
#define XLogFileName(fname, tli, logSegNo, wal_segsz_bytes) \ snprintf(fname, MAXFNAMELEN, "%08X%08X%08X", tli,
\ (uint32) ((logSegNo) / XLogSegmentsPerXLogId(wal_segsz_bytes)), \ (uint32) ((logSegNo) %
XLogSegmentsPerXLogId(wal_segsz_bytes)))
...
#define XLogFromFileName(fname, tli, logSegNo, wal_segsz_bytes) \ do {
\ uint32 log; \ uint32 seg;
\ sscanf(fname, "%08X%08X%08X", tli, &log, &seg); \ *logSegNo = (uint64) log *
XLogSegmentsPerXLogId(wal_segsz_bytes) + seg; \ } while (0)
If there's an archive_command script that simply copies WAL files
somewhere then it might overwrite old logs when filenames overlap. I
haven't surveyed all the postgres backup tools & scripts out there but
it also seems conceivable that some tools will do the equivalent of
XLogFromFileName() so that they can be aware of there are missing logs
in a recovery scenario. Those tools could conceivably get broken by
an overlapping/decremented XLogSegNo.
I haven't fully thought through replication to consider whether
anything could break there, but that's another open question.
There are a few different approaches that could be taken to determine
the next WAL sequence number.
1) simplest: increment filename's middle digit by 1, zero out the
right digit. no filename overlap, don't need to know WAL segment
size. has XLogSegNo overlap.
2) use the next valid WAL filename with segment size awareness. no
filename overlap, has XLogSegNo overlap.
3) translate old DB filename to XLogSegNo, XLogSegNo++, translate to
new DB filename. no XLogSegNo overlap, has filename overlap.
4) most complex: XLogSegNo++, translate to new DB filename, then
increase filename until it's greater than last used filename in old
db. Always has gaps, never overlaps.
I'm thinking option 4 sounds the most correct. Any thoughts from
others to the contrary?
Anything else that is worth testing to look for potential problems
after pg_upgrade with different WAL segment sizes?
-Jeremy
--
http://about.me/jeremy_schneider
Hi, On 2017-11-10 15:46:11 +0000, Bossart, Nathan wrote: > The thread for the recent change to allow setting the WAL segment size at > initdb time [0] included a bit of discussion regarding pg_upgrade [1], > where it was suggested that relaxing an error check (presumably in > check_control_data()) might be enough to upgrade servers to a different WAL > segment size. > > We've had success with our initial testing of upgrades to larger WAL > segment sizes, including post-upgrade pgbench runs. Beyond adjusting > check_control_data(), it looks like the 'pg_resetwal -l' call in > copy_xact_xlog_xid() may need to be adjusted to ensure that the WAL > starting address is set to a valid value. > > Allowing changes to the WAL segment size during pg_upgrade seems like a > nice way to avoid needing a dump and load, so I would like to propose > adding support for this. I'd be happy to submit patches for this in the > next commitfest. Hm. I'm not really on-board with doing this in pg_upgrade. A more logical place seems to be pg_resetwal or something - there's no need to force a pg_upgrade cycle (which is pretty expensive on clusters with a significant number of objects) for somebody that wants to change the segment size of a cluster without changing the major version. pg_resetwal or a new tool seems like a more appropriate places for this. Greetings, Andres Freund
On Tue, Nov 14, 2017 at 6:11 AM, Andres Freund <andres@anarazel.de> wrote: > Hm. I'm not really on-board with doing this in pg_upgrade. A more > logical place seems to be pg_resetwal or something - there's no need to > force a pg_upgrade cycle (which is pretty expensive on clusters with a > significant number of objects) for somebody that wants to change the > segment size of a cluster without changing the major version. > pg_resetwal or a new tool seems like a more appropriate places for this. pg_upgrade makes use of pg_resetwal, so I am assuming that what Nathan means is actually what you mean, so as pg_upgrade gains as well an option with the segment size which will wrap the pg_resetwal's call. -- Michael
On 2017-11-14 07:26:22 +0900, Michael Paquier wrote: > On Tue, Nov 14, 2017 at 6:11 AM, Andres Freund <andres@anarazel.de> wrote: > > Hm. I'm not really on-board with doing this in pg_upgrade. A more > > logical place seems to be pg_resetwal or something - there's no need to > > force a pg_upgrade cycle (which is pretty expensive on clusters with a > > significant number of objects) for somebody that wants to change the > > segment size of a cluster without changing the major version. > > pg_resetwal or a new tool seems like a more appropriate places for this. > > pg_upgrade makes use of pg_resetwal, so I am assuming that what Nathan > means is actually what you mean, so as pg_upgrade gains as well an > option with the segment size which will wrap the pg_resetwal's call. Even if that's the case, I fail to see why it'd be a good idea to have any sort of pg_upgrade integration here. We should make pg_resetwal's checks for this good enough, and not conflate something unrelated with pg_upgrade goals. Greetings, Andres Freund
On Tue, Nov 14, 2017 at 7:32 AM, Andres Freund <andres@anarazel.de> wrote: > On 2017-11-14 07:26:22 +0900, Michael Paquier wrote: >> On Tue, Nov 14, 2017 at 6:11 AM, Andres Freund <andres@anarazel.de> wrote: >> > Hm. I'm not really on-board with doing this in pg_upgrade. A more >> > logical place seems to be pg_resetwal or something - there's no need to >> > force a pg_upgrade cycle (which is pretty expensive on clusters with a >> > significant number of objects) for somebody that wants to change the >> > segment size of a cluster without changing the major version. >> > pg_resetwal or a new tool seems like a more appropriate places for this. >> >> pg_upgrade makes use of pg_resetwal, so I am assuming that what Nathan >> means is actually what you mean, so as pg_upgrade gains as well an >> option with the segment size which will wrap the pg_resetwal's call. > > Even if that's the case, I fail to see why it'd be a good idea to have > any sort of pg_upgrade integration here. We should make pg_resetwal's > checks for this good enough, and not conflate something unrelated with > pg_upgrade goals. Both positions can be defended. Note that some users like to have the upgrade experience within one single command do as much as possible if possible, and this may include the possibility to switch segment size to make the tool more friendly. I definitely agree with your point to make the low-level magic happen in pg_resetwal though. Having pg_upgrade call that at will could be argued afterwards. -- Michael
Michael Paquier <michael.paquier@gmail.com> writes:
> On Tue, Nov 14, 2017 at 7:32 AM, Andres Freund <andres@anarazel.de> wrote:
>> Even if that's the case, I fail to see why it'd be a good idea to have
>> any sort of pg_upgrade integration here. We should make pg_resetwal's
>> checks for this good enough, and not conflate something unrelated with
>> pg_upgrade goals.
> Both positions can be defended. Note that some users like to have the
> upgrade experience within one single command do as much as possible if
> possible, and this may include the possibility to switch segment size
> to make the tool more friendly. I definitely agree with your point to
> make the low-level magic happen in pg_resetwal though. Having
> pg_upgrade call that at will could be argued afterwards.
FWIW, I agree with Andres' position here. I think the charter of
pg_upgrade is to duplicate the old cluster as closely as it can,
not to modify its configuration. A close analogy is that it does not
attempt to upgrade extension versions while migrating the cluster.
regards, tom lane
On 11/13/17, 5:09 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: > Michael Paquier <michael.paquier@gmail.com> writes: >> On Tue, Nov 14, 2017 at 7:32 AM, Andres Freund <andres@anarazel.de> wrote: >>> Even if that's the case, I fail to see why it'd be a good idea to have >>> any sort of pg_upgrade integration here. We should make pg_resetwal's >>> checks for this good enough, and not conflate something unrelated with >>> pg_upgrade goals. >> >> Both positions can be defended. Note that some users like to have the >> upgrade experience within one single command do as much as possible if >> possible, and this may include the possibility to switch segment size >> to make the tool more friendly. I definitely agree with your point to >> make the low-level magic happen in pg_resetwal though. Having >> pg_upgrade call that at will could be argued afterwards. > > FWIW, I agree with Andres' position here. I think the charter of > pg_upgrade is to duplicate the old cluster as closely as it can, > not to modify its configuration. A close analogy is that it does not > attempt to upgrade extension versions while migrating the cluster. Fair points. If we added an option to pg_resetwal, should we bother trying to handle the WAL filename overlap that Jeremy mentioned? The -l option gives us the ability to set the WAL starting address manually, but it might not be terribly clear to end users that this is something to watch out for. Nathan
On Tue, Nov 14, 2017 at 8:38 AM, Bossart, Nathan <bossartn@amazon.com> wrote: > Fair points. If we added an option to pg_resetwal, should we bother > trying to handle the WAL filename overlap that Jeremy mentioned? The > -l option gives us the ability to set the WAL starting address > manually, but it might not be terribly clear to end users that this is > something to watch out for. After running an upgrade the previous WAL segments become useless, and that's the same when changing a segment size. Even if you take care of having the pg_resetwal-ed cluster using a segment name ahead of what the previous cluster is using, you still run after the risk of having other nodes with the previous segment size overwrite the WAL segments of the new size. In short, that's only a matter of being careful with your archive locations. -- Michael
On Mon, Nov 13, 2017 at 3:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> On Tue, Nov 14, 2017 at 7:32 AM, Andres Freund <andres@anarazel.de> wrote: >>> Even if that's the case, I fail to see why it'd be a good idea to have >>> any sort of pg_upgrade integration here. We should make pg_resetwal's >>> checks for this good enough, and not conflate something unrelated with >>> pg_upgrade goals. > > FWIW, I agree with Andres' position here. I think the charter of > pg_upgrade is to duplicate the old cluster as closely as it can, > not to modify its configuration. A close analogy is that it does not > attempt to upgrade extension versions while migrating the cluster. Having pg_upgrade simply allow an upgrade where the WAL sizes don't match between the old cluster and the new cluster seems fairly trivial. pg_upgrade isn't changing anything; it's just *not* requiring the new and old clusters to match in this way. I'll admit I'm much newer to postgresql than everyone else on this list, but I haven't yet thought of any technical reason this check is actually needed. (Just the WAL sequencing/naming concern I outlined earlier.) Writing code to change the WAL size on an existing cluster will be a little more complex. We will need to delete all WAL files and recreate them at the new sizes. We will need to either (1) be absolutely sure that there was a clean shutdown or (2) copy WAL data from the old files to the new files. We will need to think harder through the issue of gaps being introduced in the sequence of WAL filenames and effects on backup/recovery. These two ideas don't seem mutually exclusive to me. If pg_upgrade allows working with different WAL sizes, it doesn't mean we can't still introduce a utility to change the WAL size on running clusters. So yeah - having a utility command to change the WAL size on a running cluster is a great idea. But why are we wanting to maintain a check in pg_upgrade which doesn't actually seem technically necessary? Or am I missing something that would break if the WAL sizes didn't match across two clusters when pg_upgrade moved the data between them? -Jeremy -- http://about.me/jeremy_schneider (Disclaimer: I work for AWS and my opinions are my own.)
On Fri, Nov 17, 2017 at 5:46 PM, Jeremy Schneider <schneider@ardentperf.com> wrote: > Having pg_upgrade simply allow an upgrade where the WAL sizes don't match > between the old cluster and the new cluster seems fairly trivial. ... > Writing code to change the WAL size on an existing cluster will be a little more > complex. We will need to delete all WAL files and recreate them at the new > sizes. We will need to either (1) be absolutely sure that there was a > clean shutdown > or (2) copy WAL data from the old files to the new files. I think pg_resetwal has most of this logic already. > These two ideas don't seem mutually exclusive to me. If pg_upgrade > allows working > with different WAL sizes, it doesn't mean we can't still introduce a > utility to change the > WAL size on running clusters. I don't think anyone is talking about doing this on a running cluster. But it seems simple enough on a cluster that has been shut down. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Apologies for the delay. Here is a patch to allow users to change the WAL segment size of a cluster with pg_resetwal. Like the '--wal-segize' option in initdb, the new '-s' option accepts segment sizes in megabytes. This patch also fixes two small issues that I noticed. The first is a division-by-zero error caused when the control data must be guessed. If the control data is damaged, WalSegSz will not be set to the guessed WAL segment size, resulting in an error during XLogFromFileName(). The attached patch resolves this issue by ensuring WalSegSz is set to a valid value after either reading or guessing the control values. The second issue is with the automatically determined new WAL starting address calculation. Currently, the starting address is determined by looking at the last WAL file name, rounding the WAL byte position up to the next segment of the new size, and then incrementing to the next WAL segment for the new size. I believe this can cause the WAL byte position to go backwards in some cases (e.g. WAL position is towards the end of a 1024 MB segment and pg_resetwal is used to switch the segment size to 1 MB). Since the "current" WAL byte position calculated in pg_resetwal is always the beginning of the latest log, the current strategy may not yield a byte position sufficiently far ahead. The attached patch changes this logic to determine the last byte position in the latest log (for the old WAL segment size) prior to aligning to the new segment size and incrementing. Note that some segment size changes will cause old WAL file names to be reused. The new documentation for '-s' contains a note warning of this side effect. I considered a couple of strategies to prevent this, but I chose not to include any in the patch based on previous correspondence in this thread. If file name overlap is an issue, users can already use the '-l' option to set a safe WAL starting address. Nathan
Attachment
On 2/7/18 13:34, Bossart, Nathan wrote: > Here is a patch to allow users to change the WAL segment size of a cluster with > pg_resetwal. Like the '--wal-segize' option in initdb, the new '-s' option > accepts segment sizes in megabytes. Or how about we call the new option, um, --wal-segsize? > The first is a division-by-zero error caused when the control data must be > guessed. If the control data is damaged, WalSegSz will not be set to the > guessed WAL segment size, resulting in an error during XLogFromFileName(). The > attached patch resolves this issue by ensuring WalSegSz is set to a valid value > after either reading or guessing the control values. I noticed this issue in pg_controldata too. Maybe that should be combined in a separate patch. > Note that some segment size changes will cause old WAL file names to be reused. > The new documentation for '-s' contains a note warning of this side effect. I > considered a couple of strategies to prevent this, but I chose not to include > any in the patch based on previous correspondence in this thread. If file name > overlap is an issue, users can already use the '-l' option to set a safe WAL > starting address. The patch "Configurable file mode mask" contains the beginning of a test suite for pg_resetwal. It would be great if we could get a test case for this new functionality implemented. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Thanks for taking a look.
On 3/3/18, 12:22 PM, "Peter Eisentraut" <peter.eisentraut@2ndquadrant.com> wrote:
> On 2/7/18 13:34, Bossart, Nathan wrote:
>> Here is a patch to allow users to change the WAL segment size of a cluster with
>> pg_resetwal. Like the '--wal-segize' option in initdb, the new '-s' option
>> accepts segment sizes in megabytes.
>
> Or how about we call the new option, um, --wal-segsize?
It doesn't look like pg_resetwal takes any long options except for
--help and --version, although I suppose those could be added as part
of this effort.
>> The first is a division-by-zero error caused when the control data must be
>> guessed. If the control data is damaged, WalSegSz will not be set to the
>> guessed WAL segment size, resulting in an error during XLogFromFileName(). The
>> attached patch resolves this issue by ensuring WalSegSz is set to a valid value
>> after either reading or guessing the control values.
>
> I noticed this issue in pg_controldata too. Maybe that should be
> combined in a separate patch.
IIUC you are talking about the following code in pg_controldata:
/*
* Calculate name of the WAL file containing the latest checkpoint's REDO
* start point.
*/
XLByteToSeg(ControlFile->checkPointCopy.redo, segno, WalSegSz);
XLogFileName(xlogfilename, ControlFile->checkPointCopy.ThisTimeLineID,
segno, WalSegSz);
If the control file says that the WAL segment size is 0 bytes, then
XLByteToSeg() will indeed fail. I think a similar issue is still
present for XLogFromFileName() in pg_resetwal even with this patch.
For pg_controldata, perhaps the affected output ("Latest checkpoint's
REDO WAL file") should be marked unknown if the control file specifies
a 0-byte WAL segment size. For pg_resetwal, perhaps the WAL segment
size should be "guessed" in this case.
> The patch "Configurable file mode mask" contains the beginning of a test
> suite for pg_resetwal. It would be great if we could get a test case
> for this new functionality implemented.
I agree. I will take a look at that patch set.
Nathan
Here is a new set of patches that addresses most of Peter's feedback. I've split it into four pieces: 0001: Fix division-by-zero error in pg_controldata 0002: Fix division-by-zero error in pg_resetwal 0003: Allow users to specify WAL segment size in pg_resetwal 0004: Add long options to pg_resetwal (including --wal-segsize) On 3/3/18, 12:22 PM, "Peter Eisentraut" <peter.eisentraut@2ndquadrant.com> wrote: > The patch "Configurable file mode mask" contains the beginning of a test > suite for pg_resetwal. It would be great if we could get a test case > for this new functionality implemented. I haven't added a test case yet, as this thread [0] seems to still be in-progress. Nathan [0] https://postgr.es/m/ad346fe6-b23e-59f1-ecb7-0e08390ad629%40pgmasters.net
Attachment
On 3/13/18 20:53, Bossart, Nathan wrote:
> Here is a new set of patches that addresses most of Peter's feedback.
> I've split it into four pieces:
>
> 0001: Fix division-by-zero error in pg_controldata
committed that
> 0002: Fix division-by-zero error in pg_resetwal
This looks a bit more complicated than necessary to me. I think there
is a mistake in the original patch fc49e24fa69: In ReadControlFile(),
it says
/* return false if WalSegSz is not valid */
but then it doesn't actually do that.
If we make that change, then a wrong WAL segment size in the control
file would send us to GuessControlValues(). There, we need to set
WalSegSz, and everything would work.
diff --git a/src/bin/pg_resetwal/pg_resetwal.c
b/src/bin/pg_resetwal/pg_resetwal.c
index a132cf2e9f..c99e7a8db1 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -601,7 +601,7 @@ ReadControlFile(void)
fprintf(stderr,
_("%s: pg_control specifies invalid WAL segment size
(%d bytes); proceed with caution \n"),
progname, WalSegSz);
- guessed = true;
+ return false;
}
return true;
@@ -678,7 +678,7 @@ GuessControlValues(void)
ControlFile.floatFormat = FLOATFORMAT_VALUE;
ControlFile.blcksz = BLCKSZ;
ControlFile.relseg_size = RELSEG_SIZE;
- ControlFile.xlog_blcksz = XLOG_BLCKSZ;
+ WalSegSz = ControlFile.xlog_blcksz = XLOG_BLCKSZ;
ControlFile.xlog_seg_size = DEFAULT_XLOG_SEG_SIZE;
ControlFile.nameDataLen = NAMEDATALEN;
ControlFile.indexMaxKeys = INDEX_MAX_KEYS;
What do you think?
Does your patch aim to do something different?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/21/18, 12:57 PM, "Peter Eisentraut" <peter.eisentraut@2ndquadrant.com> wrote:
> On 3/13/18 20:53, Bossart, Nathan wrote:
>> 0001: Fix division-by-zero error in pg_controldata
>
> committed that
Thanks!
>> 0002: Fix division-by-zero error in pg_resetwal
>
> This looks a bit more complicated than necessary to me. I think there
> is a mistake in the original patch fc49e24fa69: In ReadControlFile(),
> it says
>
> /* return false if WalSegSz is not valid */
>
> but then it doesn't actually do that.
>
> If we make that change, then a wrong WAL segment size in the control
> file would send us to GuessControlValues(). There, we need to set
> WalSegSz, and everything would work.
>
> diff --git a/src/bin/pg_resetwal/pg_resetwal.c
> b/src/bin/pg_resetwal/pg_resetwal.c
> index a132cf2e9f..c99e7a8db1 100644
> --- a/src/bin/pg_resetwal/pg_resetwal.c
> +++ b/src/bin/pg_resetwal/pg_resetwal.c
> @@ -601,7 +601,7 @@ ReadControlFile(void)
> fprintf(stderr,
> _("%s: pg_control specifies invalid WAL segment size
> (%d bytes); proceed with caution \n"),
> progname, WalSegSz);
> - guessed = true;
> + return false;
> }
>
> return true;
> @@ -678,7 +678,7 @@ GuessControlValues(void)
> ControlFile.floatFormat = FLOATFORMAT_VALUE;
> ControlFile.blcksz = BLCKSZ;
> ControlFile.relseg_size = RELSEG_SIZE;
> - ControlFile.xlog_blcksz = XLOG_BLCKSZ;
> + WalSegSz = ControlFile.xlog_blcksz = XLOG_BLCKSZ;
> ControlFile.xlog_seg_size = DEFAULT_XLOG_SEG_SIZE;
> ControlFile.nameDataLen = NAMEDATALEN;
> ControlFile.indexMaxKeys = INDEX_MAX_KEYS;
>
> What do you think?
>
> Does your patch aim to do something different?
With my patch, I was trying to still use the rest of the values in the
control file, altering only the WAL segment size. Also, I was doing a
bit of setup for 0003, where WalSegSz is used as the "new" WAL segment
size.
However, I think your approach is better. In addition to being much
simpler, it might make more sense to treat the control file as
corrupted if the WAL segment size is invalid, anyway. Any setup for
0003 can be easily moved, too.
Nathan
Here is an updated set of patches that use the newly proposed approach for avoiding division-by-zero errors in pg_resetwal. Nathan
Attachment
On 3/21/18 17:14, Bossart, Nathan wrote: > Here is an updated set of patches that use the newly proposed approach > for avoiding division-by-zero errors in pg_resetwal. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/25/18, 2:02 PM, "Peter Eisentraut" <peter.eisentraut@2ndquadrant.com> wrote: > committed Thanks! Nathan