Horton

Thread

Commits

  1. pg_resetwal: Prevent division-by-zero errors

  2. Make WAL segment size configurable at initdb time.

  1. pg_upgrade to clusters with a different WAL segment size

    Nathan Bossart <bossartn@amazon.com> — 2017-11-10T15:46:11Z

    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

  2. Re: pg_upgrade to clusters with a different WAL segment size

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-11T00:04:21Z

    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

  3. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Jeremy Schneider <schneider@ardentperf.com> — 2017-11-13T21:00:52Z

    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

  4. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Andres Freund <andres@anarazel.de> — 2017-11-13T21:11:42Z

    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

  5. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-13T22:26:22Z

    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

  6. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Andres Freund <andres@anarazel.de> — 2017-11-13T22:32:38Z

    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

  7. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-13T23:05:13Z

    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

  8. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-11-13T23:09:04Z

    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

  9. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Nathan Bossart <bossartn@amazon.com> — 2017-11-13T23:38:06Z

    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

  10. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-14T00:23:00Z

    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

  11. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Jeremy Schneider <schneider@ardentperf.com> — 2017-11-17T22:46:42Z

    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.)

  12. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Robert Haas <robertmhaas@gmail.com> — 2017-11-17T23:26:29Z

    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

  13. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Nathan Bossart <bossartn@amazon.com> — 2018-02-07T18:34:58Z

    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

  14. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-03T18:22:01Z

    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

  15. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Nathan Bossart <bossartn@amazon.com> — 2018-03-06T15:35:13Z

    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

  16. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Nathan Bossart <bossartn@amazon.com> — 2018-03-14T00:53:08Z

    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

  17. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-21T17:57:01Z

    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

  18. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Nathan Bossart <bossartn@amazon.com> — 2018-03-21T18:35:54Z

    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

  19. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Nathan Bossart <bossartn@amazon.com> — 2018-03-21T21:14:32Z

    Here is an updated set of patches that use the newly proposed approach
for avoiding division-by-zero errors in pg_resetwal.

Nathan

  20. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-25T19:02:06Z

    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

  21. Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

    Nathan Bossart <bossartn@amazon.com> — 2018-03-26T14:05:47Z

    On 3/25/18, 2:02 PM, "Peter Eisentraut" <peter.eisentraut@2ndquadrant.com> wrote:
> committed

Thanks!

Nathan