Re: increasing the default WAL segment size

Hi,

I'd like to propose that we increase the default WAL segment size,
which is currently 16MB. It was first set to that value in commit
47937403676d913c0e740eec6b85113865c6c8ab in October of 1999; prior to
that, it was 64MB. Between 1999 and now, there have been three
significant changes that make me think it might be time to rethink
this value:

1. Transaction rates are vastly higher these days. In 1999, I think
we were still limited to ~2^32 transactions during the entire lifetime
of the server; transaction ID wraparound hadn't been invented yet.[1]
Today, some installations do that many write transactions in under a
week. The practical consequence of this is that WAL files fill up in
extremely short periods of time. Some users generate multiple
terabytes of WAL per day, which means they are generating - and very
likely archiving - WAL files a rate of greater than 1 per second!
That poses multiple problems. For example, if your archive command
happens to involve ssh, you might run into trouble because of this
sort of thing:

[rhaas pgsql]$ /usr/bin/time ssh hydra true
1.57 real 0.00 user 0.00 sys

Also, your operating system's implementation of directories and the
commands to work with them (like ls) don't necessarily scale well to
tens or hundreds of thousands of archived files.

Furthermore, there is an enforced, synchronous fsync at the end of
every segment, which actually does hurt performance on write-heavy
workloads.[2] Of course, if that were the only reason to consider
increasing the segment size, it would probably make more sense to just
try to push that extra fsync into the background, but that's not
really the case. From what I hear, the gigantic number of files is a
bigger pain point.

2. Disks are a bit larger these days. In the worst case, we waste
just under twice as much space as whatever the segment size is: you
might need 1 byte from the oldest segment you're keeping and 1 byte
from the newest segment that you are keeping, but not the remaining
contents of either file. In 1999, trying to limit disk wastage to
<32MB probably seemed reasonable, but today that's very little disk
space. I think at that time typical hard drive sizes were around 10
GB, whereas today they are around 1 TB.[3] I'm not sure whether the
size of the sorts of high-performance storage that is likely to be
used for pg_xlog has grown as fast as hard drives generally, but even
so it seems pretty clear to me that trying to limit disk wastage to
32MB is excessively conservative on modern hardware.

3. archive_timeout is no longer a frequently used option. Obviously,
if you are frequently archiving partial segments, you don't want the
segment size to be too large, because if it is, each forced segment
switch potentially wastes a large amount of space (and bandwidth).
But given streaming replication and pg_receivexlog, the use case for
archiving partial segments is, at least according to my understanding,
a lot narrower than it used to be. So, I think we don't have to worry
as much about keeping forced segment switches cheap as we did during
the 8.x series.

Considering those three factors, I think we should consider pushing
the default value up somewhat higher for v10. Reverting to the 64MB
size that we had prior to 47937403676d913c0e740eec6b85113865c6c8ab
sounds pretty reasonable. Users with really high transaction rates
might even prefer a higher value (e.g. 256MB, 1GB) but that's hardly
practical for small installs given our default of max_wal_size = 1GB.
Possibly it would make sense for this to be configurable at initdb
time instead of requiring a recompile; we probably don't save any
significant number of cycles by compiling this into the server.

Thoughts?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

[1] I believe at that time we consumed an XID even for a read-only
transaction, too; today, we can do 2^32 read transactions in a few
hours.
[2] Amit did some benchmarking on this, I believe, but I don't have
the numbers handy.
[3] https://commons.wikimedia.org/wiki/File:Hard_drive_capacity_over_time.png

On Wed, Aug 24, 2016 at 10:31 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 1. Transaction rates are vastly higher these days. In 1999, I think
> we were still limited to ~2^32 transactions during the entire lifetime
> of the server; transaction ID wraparound hadn't been invented yet.[1]
> Today, some installations do that many write transactions in under a
> week. The practical consequence of this is that WAL files fill up in
> extremely short periods of time. Some users generate multiple
> terabytes of WAL per day, which means they are generating - and very
> likely archiving - WAL files a rate of greater than 1 per second!
> That poses multiple problems. For example, if your archive command
> happens to involve ssh, you might run into trouble because of this
> sort of thing:
>
> [rhaas pgsql]$ /usr/bin/time ssh hydra true
> 1.57 real 0.00 user 0.00 sys
...
> Considering those three factors, I think we should consider pushing
> the default value up somewhat higher for v10. Reverting to the 64MB
> size that we had prior to 47937403676d913c0e740eec6b85113865c6c8ab
> sounds pretty reasonable. Users with really high transaction rates
> might even prefer a higher value (e.g. 256MB, 1GB) but that's hardly
> practical for small installs given our default of max_wal_size = 1GB.
> Possibly it would make sense for this to be configurable at initdb
> time instead of requiring a recompile; we probably don't save any
> significant number of cycles by compiling this into the server.

FWIW, +1

We're already hurt by the small segments due to a similar phenomenon
as the ssh case: TCP slow start. Designing the archive/recovery
command to work around TCP slow start is quite complex, and bigger
segments would just be a better thing.

Not to mention that bigger segments compress better.

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I'd like to propose that we increase the default WAL segment size,
> which is currently 16MB.

That seems like a reasonable thing to consider ...

> Possibly it would make sense for this to be configurable at initdb
> time instead of requiring a recompile;

... but I think this is just folly. You'd have to do major amounts
of work to keep, eg, slave servers on the same page as the master
about what the segment size is. Better to keep treating it like
BLCKSZ, as a fixed parameter of a build. (There's a reason why we
keep this number in pg_control.)

regards, tom lane

> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Robert Haas
> Considering those three factors, I think we should consider pushing the
> default value up somewhat higher for v10. Reverting to the 64MB size that
> we had prior to 47937403676d913c0e740eec6b85113865c6c8ab
> sounds pretty reasonable.

+1
The other downside is that the response time of transactions may degrade when they have to wait for a new WAL segment to be created. Tha might pop up as occasional slow or higher maximum response time, which is a mystery to users. Maybe it's time to use posix_fallocate() to create WAL segments.

> Possibly it would make sense for this to be configurable at initdb time
> instead of requiring a recompile; we probably don't save any significant
> number of cycles by compiling this into the server.

+1

> 3. archive_timeout is no longer a frequently used option. Obviously, if
> you are frequently archiving partial segments, you don't want the segment
> size to be too large, because if it is, each forced segment switch
> potentially wastes a large amount of space (and bandwidth).
> But given streaming replication and pg_receivexlog, the use case for
> archiving partial segments is, at least according to my understanding, a
> lot narrower than it used to be. So, I think we don't have to worry as
> much about keeping forced segment switches cheap as we did during the 8.x
> series.

I'm not sure about this. I know (many or not) users use continuous archiving with archive_command and archive_timeout for backups, and don't want to use streaming replication, because the system is not worth the cost and trouble of HA. I heard from a few users that they were surprised when they knew that PostgreSQL generates WAL even when no update transaction is happening. Is this still true?

Regards
Takayuki Tsunakawa

On 2016-08-24 22:33:49 -0400, Tom Lane wrote:
> > Possibly it would make sense for this to be configurable at initdb
> > time instead of requiring a recompile;
>
> ... but I think this is just folly. You'd have to do major amounts
> of work to keep, eg, slave servers on the same page as the master
> about what the segment size is.

Don't think it'd actually be all that complicated, we already verify
the compatibility of some things. But I'm doubtful it's worth it, and
I'm also rather doubtful that it's actually without overhead.

Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2016-08-24 22:33:49 -0400, Tom Lane wrote:
>> ... but I think this is just folly. You'd have to do major amounts
>> of work to keep, eg, slave servers on the same page as the master
>> about what the segment size is.

> Don't think it'd actually be all that complicated, we already verify
> the compatibility of some things. But I'm doubtful it's worth it, and
> I'm also rather doubtful that it's actually without overhead.

My point is basically that it'll introduce failure modes that we don't
currently concern ourselves with. Yes, you can do configure
--with-wal-segsize, but it's on your own head whether the resulting build
will interoperate with anything else --- and I'm quite sure nobody tests,
eg, walsender or walreceiver to see if they fail sanely in such cases.
I don't think we'd get to take such a laissez-faire position with respect
to an initdb option.

regards, tom lane

On Wed, Aug 24, 2016 at 10:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> ... but I think this is just folly. You'd have to do major amounts
> of work to keep, eg, slave servers on the same page as the master
> about what the segment size is.

I said an initdb-time parameter, meaning not capable of being changed
within the lifetime of the cluster. So I don't see how the slave
servers would get out of sync?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Aug 24, 2016 at 10:54 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-08-24 22:33:49 -0400, Tom Lane wrote:
>> > Possibly it would make sense for this to be configurable at initdb
>> > time instead of requiring a recompile;
>>
>> ... but I think this is just folly. You'd have to do major amounts
>> of work to keep, eg, slave servers on the same page as the master
>> about what the segment size is.
>
> Don't think it'd actually be all that complicated, we already verify
> the compatibility of some things. But I'm doubtful it's worth it, and
> I'm also rather doubtful that it's actually without overhead.

Really? Where do you think the overhead would come from? What sort
of test would you run to try to detect it?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Aug 24, 2016 at 11:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
>> On 2016-08-24 22:33:49 -0400, Tom Lane wrote:
>>> ... but I think this is just folly. You'd have to do major amounts
>>> of work to keep, eg, slave servers on the same page as the master
>>> about what the segment size is.
>
>> Don't think it'd actually be all that complicated, we already verify
>> the compatibility of some things. But I'm doubtful it's worth it, and
>> I'm also rather doubtful that it's actually without overhead.
>
> My point is basically that it'll introduce failure modes that we don't
> currently concern ourselves with. Yes, you can do configure
> --with-wal-segsize, but it's on your own head whether the resulting build
> will interoperate with anything else --- and I'm quite sure nobody tests,
> eg, walsender or walreceiver to see if they fail sanely in such cases.
> I don't think we'd get to take such a laissez-faire position with respect
> to an initdb option.

I am really confused by this. If you connect a slave to a master
other than the one that you cloned to create the salve, of course
that's going to fail. But if the slave is cloned from the master,
then the segment size is going to match. It seems like the only thing
we need to do to make this work is make sure to get the segment size
from the control file rather than anywhere else, which doesn't seem
very difficult. What am I missing?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Aug 24, 2016 at 10:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> ... but I think this is just folly. You'd have to do major amounts
>> of work to keep, eg, slave servers on the same page as the master
>> about what the segment size is.

> I said an initdb-time parameter, meaning not capable of being changed
> within the lifetime of the cluster. So I don't see how the slave
> servers would get out of sync?

The point is that that now becomes something to worry about. I do not
think I have to exhibit a live bug within five minutes' thought before
saying that it's a risk area. It's something that we simply have not
worried about before, and IME that generally means there's some squishy
things there.

regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> What am I missing?

Maybe nothing. But I'll point out that of the things that can currently
be configured at initdb time, such as LC_COLLATE, there is not one single
one that matters to walsender/walreceiver. If you think there is zero
risk involved in introducing a parameter that will matter at that level,
you have a different concept of risk than I do.

If you'd presented some positive reason why we ought to be taking some
risk here, I'd be on board. But you haven't really. The current default
value for this parameter is nearly old enough to vote; how is it that
we suddenly need to make it easily configurable? Let's just change
the value and be happy.

regards, tom lane

On 2016-08-24 23:26:51 -0400, Robert Haas wrote:
> On Wed, Aug 24, 2016 at 10:54 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > and I'm also rather doubtful that it's actually without overhead.
>
> Really? Where do you think the overhead would come from?

ATM we do a math involving XLOG_BLCKSZ in a bunch of places (including
doing a lot of %). Some of that happens with exclusive lwlocks held, and
some even with a spinlock held IIRC. Making that variable won't be
free. Whether it's actually measurabel - hard to say. I do remember
Heikki fighting hard to simplify some parts of the critical code during
xlog scalability stuff, and that that even involved moving minor amounts
of math out of critical sections.

> What sort of test would you run to try to detect it?

Xlog scalability tests (parallel copy, parallel inserts...), and
decoding speed (pg_xlogdump --stats?)

On Wed, Aug 24, 2016 at 11:41 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> What am I missing?
>
> Maybe nothing. But I'll point out that of the things that can currently
> be configured at initdb time, such as LC_COLLATE, there is not one single
> one that matters to walsender/walreceiver. If you think there is zero
> risk involved in introducing a parameter that will matter at that level,
> you have a different concept of risk than I do.
>
> If you'd presented some positive reason why we ought to be taking some
> risk here, I'd be on board. But you haven't really. The current default
> value for this parameter is nearly old enough to vote; how is it that
> we suddenly need to make it easily configurable? Let's just change
> the value and be happy.

I certainly think that's a good first cut. As I said before, I think
that increasing the value from 16MB to 64MB won't really hurt people
with mostly-default configurations. max_wal_size=1GB currently means
64 16-MB segments; if it starts meaning 16 64-MB segments, I don't
think that will have much impact on on people one way or the other.
Meanwhile, we'll significantly help people who are currently
generating painfully large but not totally insane numbers of WAL
segments. Someone who is currently generating 32,768 WAL segments per
day - about one every 2.6 seconds - will have a significantly easier
time if they start generating 8,192 WAL segments per day - about one
every 10.5 seconds - instead. It's just much easier for a reasonably
simple archive command to keep up, "ls" doesn't have as many directory
entries to sort, etc.

However, for people who have really high velocity systems - say
300,000 WAL segments per day - a fourfold increase in the segment size
only gets them down to 75,000 WAL segments per day, which is still
pretty nuts. High tens of thousands of segments per day is, surely,
easier to manage than low hundreds of thousands, but it still puts
really tight requirements on how fast your archive_command has to run.
On that kind of system, you really want a segment size of maybe 1GB.
In this example that gets you down to ~4700 WAL files per day, or
about one every 18 seconds. But 1GB is clearly too large to be the
default.

I think we're going to run into this issue more and more as people
start running PostgreSQL on larger databases. In current releases,
the cost of wraparound autovacuums can easily be the limiting factor
here: the I/O cost is proportional to the XID burn rate multiplied by
the entire size of the database. So mostly read-only databases or
databases that only take batch loads can be fine even if they are
really big, but it's hard to scale databases that do lots of
transaction processing beyond a certain size because you just end up
running continuous wraparound vacuums and eventually you can't even do
that fast enough. The freeze map changes in 9.6 should help with this
problem, though, at least for databases that have hot spots rather
than uniform access, which is of course very common. I think the
result of that is likely to be that people try to scale up PostgreSQL
to larger databases than ever before. New techniques for indexing
large amounts of data (like BRIN) and for querying it (like parallel
query, especially once we support having the driving scan be a bitmap
heap scan) are going to encourage people in that direction, too.
You're asking why we suddenly need to make this configurable as if it
were a surprising need, but I think it would be more surprising if
scaling up didn't create some new needs. I can't think of any reason
why a 100TB database and a 100MB database should both want to use the
same WAL segment size, and I think we want to support both of those
things.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Aug 24, 2016 at 11:52 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-08-24 23:26:51 -0400, Robert Haas wrote:
>> On Wed, Aug 24, 2016 at 10:54 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> > and I'm also rather doubtful that it's actually without overhead.
>>
>> Really? Where do you think the overhead would come from?
>
> ATM we do a math involving XLOG_BLCKSZ in a bunch of places (including
> doing a lot of %). Some of that happens with exclusive lwlocks held, and
> some even with a spinlock held IIRC. Making that variable won't be
> free. Whether it's actually measurabel - hard to say. I do remember
> Heikki fighting hard to simplify some parts of the critical code during
> xlog scalability stuff, and that that even involved moving minor amounts
> of math out of critical sections.

OK, that's helpful context.

>> What sort of test would you run to try to detect it?
>
> Xlog scalability tests (parallel copy, parallel inserts...), and
> decoding speed (pg_xlogdump --stats?)

Thanks; that's helpful, too.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-08-25 00:28:58 -0400, Robert Haas wrote:
> On Wed, Aug 24, 2016 at 11:52 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2016-08-24 23:26:51 -0400, Robert Haas wrote:
> >> On Wed, Aug 24, 2016 at 10:54 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >> > and I'm also rather doubtful that it's actually without overhead.
> >>
> >> Really? Where do you think the overhead would come from?
> >
> > ATM we do a math involving XLOG_BLCKSZ in a bunch of places (including
> > doing a lot of %). Some of that happens with exclusive lwlocks held, and
> > some even with a spinlock held IIRC. Making that variable won't be
> > free. Whether it's actually measurabel - hard to say. I do remember
> > Heikki fighting hard to simplify some parts of the critical code during
> > xlog scalability stuff, and that that even involved moving minor amounts
> > of math out of critical sections.
>
> OK, that's helpful context.
>
> >> What sort of test would you run to try to detect it?
> >
> > Xlog scalability tests (parallel copy, parallel inserts...), and
> > decoding speed (pg_xlogdump --stats?)
>
> Thanks; that's helpful, too.

FWIW, I'm also doubtful that investing time into making this initdb
configurable is a good use of time: The number of users that'll adjust
initdb time parameters is going to be fairly small.

On Thu, Aug 25, 2016 at 12:35 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> FWIW, I'm also doubtful that investing time into making this initdb
> configurable is a good use of time: The number of users that'll adjust
> initdb time parameters is going to be fairly small.

I have to admit that I was skeptical about the idea of doing anything
about this at all the first few times it came up. 16MB ought to be
good enough for anyone! However, the time between beatings has now
gotten short enough that the bruises don't have time to heal before
the next beating arrives from a completely different customer. I try
not to hold my views so firmly as to be impervious to contrary
evidence.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hello hackers,
I'm no PG hacker, so maybe I'm completely wrong, so sorry if I have wasted your time. I try to make the best out of Tom Lanes comment.

What would happen if there's a database on a server with initdb (or whatever) parameter -with-wal-size=64MB and later someone decides to make it the master in a replicated system and has a slave without that parameter? Would the slave work with the "different" wal size of the master? How could be guaranteed that in such a scenario the replication either works correctly or failes with a meaningful error message?

But in general I thing a more flexible WAL size is a good idea.
To answer Andres: You have found one of the (few?) users to adjust initdb parameters.

Regards

Robert Haas <robertmhaas(at)gmail(dot)com> schrieb am 6:43 Donnerstag, 25.August 2016:

On Thu, Aug 25, 2016 at 12:35 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> FWIW, I'm also doubtful that investing time into making this initdb
> configurable is a good use of time: The number of users that'll adjust
> initdb time parameters is going to be fairly small.

I have to admit that I was skeptical about the idea of doing anything
about this at all the first few times it came up.  16MB ought to be
good enough for anyone!  However, the time between beatings has now
gotten short enough that the bruises don't have time to heal before
the next beating arrives from a completely different customer.  I try
not to hold my views so firmly as to be impervious to contrary
evidence.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Aug 24, 2016 at 10:40:06PM -0300, Claudio Freire wrote:
> > time instead of requiring a recompile; we probably don't save any
> > significant number of cycles by compiling this into the server.
>
> FWIW, +1
>
> We're already hurt by the small segments due to a similar phenomenon
> as the ssh case: TCP slow start. Designing the archive/recovery
> command to work around TCP slow start is quite complex, and bigger
> segments would just be a better thing.
>
> Not to mention that bigger segments compress better.

This would be good time to rename pg_xlog and pg_clog directories too.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On Thu, Aug 25, 2016 at 1:04 AM, Wolfgang Wilhelm
<wolfgang20121964(at)yahoo(dot)de> wrote:
> What would happen if there's a database on a server with initdb (or
> whatever) parameter -with-wal-size=64MB and later someone decides to make it
> the master in a replicated system and has a slave without that parameter?
> Would the slave work with the "different" wal size of the master? How could
> be guaranteed that in such a scenario the replication either works correctly
> or failes with a meaningful error message?

You make reference to an "initdb (or whatever) parameter" but actually
there is a big difference between the "initdb" case and the "whatever"
case. If the parameter is fixed at initdb time, then the master and
the slave will definitely agree: the slave had to be created by
copying the master, and that means the control file that contains the
size was also copied. Neither can have been changed afterwards.
That's what an initdb-time parameter means. On the other hand, if the
parameter is, say, a GUC, then you would have exactly the kinds of
problems that you are talking about here. I am not keen to solve any
of those problems, which is why I am not proposing to go any further
than an initdb-time parameter.

> But in general I thing a more flexible WAL size is a good idea.
> To answer Andres: You have found one of the (few?) users to adjust initdb
> parameters.

Good to know, thanks.

In further defense of the idea that making this more configurable
isn't nuts, it's worth noting that the history here is:

* When Vadim originally added XLogSegSize in
30659d43eb73272e20f2eb1d785a07ba3b553ed8 (September 1999), it was a
constant.
* In c3c09be34b6b0d7892f1087a23fc6eb93f3c4f04 (February 2004), this
became configurable via pg_config_manual.h.
* In cf9f6c8d8e9df28f3fbe1850ca7f042b2c01252e (May 2008), Tom made
this configurable via configure.

So there's a well-established history of making this gradually easier
for users to change.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Aug 25, 2016 at 5:32 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Wed, Aug 24, 2016 at 10:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> ... but I think this is just folly. You'd have to do major amounts
> >> of work to keep, eg, slave servers on the same page as the master
> >> about what the segment size is.
>
> > I said an initdb-time parameter, meaning not capable of being changed
> > within the lifetime of the cluster. So I don't see how the slave
> > servers would get out of sync?
>
> The point is that that now becomes something to worry about. I do not
> think I have to exhibit a live bug within five minutes' thought before
> saying that it's a risk area. It's something that we simply have not
> worried about before, and IME that generally means there's some squishy
> things there.
>

If we ignore the possible performance implications (which we shouldn't, of
course, but for the sake of argument), I think having it as a configurable
parameter in initdb would make it *less* of something to worry about.

Because it comes with the cluster during replication. I think it's more
likely that you accidentally end up with two instances compiled with
different values than that you get an issue from this.

That said, I think it also has to be a *very* bad painpoint for somebody to
care about changing it if it requires recompilation. The vast majority of
users run the packaged versions, and they don't want to run anything else.
So you will have whatever the RPMs or the DEBs or installers pick for you.
Anything that is a ./configure-time option,is something we should expect
almost nobody to change.

Changing the default will of course help/hurt those as well. But if we
change the default to something high and say "hey those of you who just run
it on a smaller system should recompile with a different --configure", we
are being *very* user-unfriendly. Or the other way around.

That doesn't mean we shouldn't change the default. We just need to be a lot
more careful about what we change it to if it's ./configure to reset it.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Robert,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> Meanwhile, we'll significantly help people who are currently
> generating painfully large but not totally insane numbers of WAL
> segments. Someone who is currently generating 32,768 WAL segments per
> day - about one every 2.6 seconds - will have a significantly easier
> time if they start generating 8,192 WAL segments per day - about one
> every 10.5 seconds - instead. It's just much easier for a reasonably
> simple archive command to keep up, "ls" doesn't have as many directory
> entries to sort, etc.

I'm generally on-board with increasing the WAL segment size, and I can
see the point that we might want to make it more easily configurable as
it's valuable to set it differently on a small database vs. a large
database, but I take exception with the notion that a "simple archive
command" is ever appropriate. Heikki's excellent talk at PGCon '15
(iirc) goes over why our archive command example is about as terrible as
you can get and that's primairly because it's just a simple 'cp'.

archive_command needs to be doing things like fsync'ing the WAL file
after it's been copied away, probably fsync'ing the directory the WAL
file has been copied into, returning the correct exit code to PG, etc.

Thankfully, there are backup/WAL archive utilities which do this
correctly and are even built to handle a large rate of WAL files for
high transaction systems (including keeping open a long-running ssh/TCP
to address the startup costs of both). Switching to 64MB would still be
nice to simply reduce the number of files you have to deal with, and I'm
all for it for that reason, but the ssh/TCP startup cost reasons aren't
good ones for the switch as people shouldn't be using a "simple" command
anyway and the good tools for WAL archiving have already addressed those
issues.

Thanks!

Stephen

On Thu, Aug 25, 2016 at 9:48 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> Meanwhile, we'll significantly help people who are currently
>> generating painfully large but not totally insane numbers of WAL
>> segments. Someone who is currently generating 32,768 WAL segments per
>> day - about one every 2.6 seconds - will have a significantly easier
>> time if they start generating 8,192 WAL segments per day - about one
>> every 10.5 seconds - instead. It's just much easier for a reasonably
>> simple archive command to keep up, "ls" doesn't have as many directory
>> entries to sort, etc.
>
> I'm generally on-board with increasing the WAL segment size, and I can
> see the point that we might want to make it more easily configurable as
> it's valuable to set it differently on a small database vs. a large
> database, but I take exception with the notion that a "simple archive
> command" is ever appropriate.

My point wasn't really that archive_command should actually be simple.
My point was that if it's being run multiple times per second, there
are additional challenges that wouldn't arise if it were being run
only every 5-10 seconds.

I guess I should have said "simpler" rather than "reasonably simple",
because there's nothing simple about setting archive_command properly.
I mean, it could only actually be simple if somebody had a good a
backup tool that provided an archive_command that you could just drop
in place. But I'm sure if somebody had such a tool, they'd take every
opportunity to bring it up, so we doubtless would have heard about it
by now. Right? :-)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Thu, Aug 25, 2016 at 9:48 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> >> Meanwhile, we'll significantly help people who are currently
> >> generating painfully large but not totally insane numbers of WAL
> >> segments. Someone who is currently generating 32,768 WAL segments per
> >> day - about one every 2.6 seconds - will have a significantly easier
> >> time if they start generating 8,192 WAL segments per day - about one
> >> every 10.5 seconds - instead. It's just much easier for a reasonably
> >> simple archive command to keep up, "ls" doesn't have as many directory
> >> entries to sort, etc.
> >
> > I'm generally on-board with increasing the WAL segment size, and I can
> > see the point that we might want to make it more easily configurable as
> > it's valuable to set it differently on a small database vs. a large
> > database, but I take exception with the notion that a "simple archive
> > command" is ever appropriate.
>
> My point wasn't really that archive_command should actually be simple.
> My point was that if it's being run multiple times per second, there
> are additional challenges that wouldn't arise if it were being run
> only every 5-10 seconds.

My point was that the concerns about TCP/ssh startup costs, which was
part of your point #1 in your initial justification for the change,
have been addressed through tooling.

> I guess I should have said "simpler" rather than "reasonably simple",
> because there's nothing simple about setting archive_command properly.

Agreed.

> I mean, it could only actually be simple if somebody had a good a
> backup tool that provided an archive_command that you could just drop
> in place. But I'm sure if somebody had such a tool, they'd take every
> opportunity to bring it up, so we doubtless would have heard about it
> by now. Right? :-)

Thankfully there's actually multiple good open source and freely
available tools that address this issue (albeit, through different
mechanisms).

Thanks!

Stephen

On Wed, Aug 24, 2016 at 08:52:20PM -0700, Andres Freund wrote:
> On 2016-08-24 23:26:51 -0400, Robert Haas wrote:
> > On Wed, Aug 24, 2016 at 10:54 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > and I'm also rather doubtful that it's actually without overhead.
> >
> > Really? Where do you think the overhead would come from?
>
> ATM we do a math involving XLOG_BLCKSZ in a bunch of places (including
> doing a lot of %). Some of that happens with exclusive lwlocks held, and
> some even with a spinlock held IIRC. Making that variable won't be
> free. Whether it's actually measurabel - hard to say. I do remember
> Heikki fighting hard to simplify some parts of the critical code during
> xlog scalability stuff, and that that even involved moving minor amounts
> of math out of critical sections.

I think Robert made a good case that high-volume servers might want a
larger WAL segment size, but as Andres pointed out, there are
performance concerns. Those might be minimized by requiring the segment
size to be a 2x multiple of 16MB.

Another issue is that many users are coming from database products that
have significant performance hits in switching WAL files so they might
be tempted to set very high segment sizes in inappropriate cases.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On Thu, Aug 25, 2016 at 10:34 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> My point wasn't really that archive_command should actually be simple.
>> My point was that if it's being run multiple times per second, there
>> are additional challenges that wouldn't arise if it were being run
>> only every 5-10 seconds.
>
> My point was that the concerns about TCP/ssh startup costs, which was
> part of your point #1 in your initial justification for the change,
> have been addressed through tooling.

It's good to know that some tool sets have addressed that, but I'm
pretty certain that not every tool set has done so, probably not even
all of the ones in common use. Anyway, I think the requirements we
impose on archive_command today are just crazy. All other things
being equal, changes that make it easier to write a decent one are
IMHO going in the right direction.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Aug 25, 2016 at 10:39 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Another issue is that many users are coming from database products that
> have significant performance hits in switching WAL files so they might
> be tempted to set very high segment sizes in inappropriate cases.

Well, we have some hit there, too. It may be smaller, but it's
certainly not zero.

I'm generally in favor of preventing people from setting ridiculous
values for settings; we shouldn't let somebody set the WAL segment
size to 8kB or something silly like that. But it's more important to
enable legitimate uses than it is to prohibit inappropriate uses. If
a particular value of a particular setting may be legitimately useful
to some users, we should allow it, even if some other user might
choose that value under false assumptions.

In short, let's eschew nannyism.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Aug 25, 2016 at 9:34 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Because it comes with the cluster during replication. I think it's more
> likely that you accidentally end up with two instances compiled with
> different values than that you get an issue from this.

I hadn't thought about it that way, but I think you're right.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 25 August 2016 at 02:31, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> Furthermore, there is an enforced, synchronous fsync at the end of
> every segment, which actually does hurt performance on write-heavy
> workloads.[2] Of course, if that were the only reason to consider
> increasing the segment size, it would probably make more sense to just
> try to push that extra fsync into the background, but that's not
> really the case. From what I hear, the gigantic number of files is a
> bigger pain point.

I think we should fully describe the problem before finding a solution.

This is too big a change to just tweak a value without discussing the
actual issue.

And if the problem is as described, how can a change of x4 be enough
to make it worth the pain of change? I think you're already admitting
it can't be worth it by discussing initdb configuration.

If we do have the pain of change, should we also consider making WAL
files variable length? What do we gain by having the files all the
same size? ISTM better to have WAL files that vary in length up to 1GB
in size.

(This is all about XLOG_SEG_SIZE; I presume XLOG_BLCKSZ can stay as it
is, right?)

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Thu, Aug 25, 2016 at 10:34 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >> My point wasn't really that archive_command should actually be simple.
> >> My point was that if it's being run multiple times per second, there
> >> are additional challenges that wouldn't arise if it were being run
> >> only every 5-10 seconds.
> >
> > My point was that the concerns about TCP/ssh startup costs, which was
> > part of your point #1 in your initial justification for the change,
> > have been addressed through tooling.
>
> It's good to know that some tool sets have addressed that, but I'm
> pretty certain that not every tool set has done so, probably not even
> all of the ones in common use. Anyway, I think the requirements we
> impose on archive_command today are just crazy. All other things
> being equal, changes that make it easier to write a decent one are
> IMHO going in the right direction.

Agreed, but, unfortunately, this isn't an "all other things being equal"
case, or we wouldn't be having this discussion. Increasing the WAL
segment size means it'll be longer before archive_command is called
which means there's a larger amount of potential data loss for users who
are using it without any other archiving/replication solution, along
with the other concerns about it possibly resulting in a higher disk
space cost.

I agree that increasing it makes sense and that 64MB is a good number,
but I wouldn't want to go much higher than that. That doesn't
completely solve the TCP/SSH start-up cost penalty as there will be
environments where that is still expensive even with 64MB WAL segments,
but it will certainly be reduced.

To try to summarize, I don't think we should be trying to solve the
TCP/SSH start-up penalty issue for all users by encouraging them to
increase the WAL segment size, at least not without covering the
trade-offs. That isn't to say we shouldn't change the default, I agree
that we should, but I believe we should keep it a reasonably
conservative change and if we make it user-configurable then we need
to be sure to document the trade-offs.

Thanks!

Stephen

On Thu, Aug 25, 2016 at 12:21 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 25 August 2016 at 02:31, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> Furthermore, there is an enforced, synchronous fsync at the end of
>> every segment, which actually does hurt performance on write-heavy
>> workloads.[2] Of course, if that were the only reason to consider
>> increasing the segment size, it would probably make more sense to just
>> try to push that extra fsync into the background, but that's not
>> really the case. From what I hear, the gigantic number of files is a
>> bigger pain point.
>
> I think we should fully describe the problem before finding a solution.
>
> This is too big a change to just tweak a value without discussing the
> actual issue.
>
> And if the problem is as described, how can a change of x4 be enough
> to make it worth the pain of change? I think you're already admitting
> it can't be worth it by discussing initdb configuration.
>
> If we do have the pain of change, should we also consider making WAL
> files variable length? What do we gain by having the files all the
> same size? ISTM better to have WAL files that vary in length up to 1GB
> in size.
>
> (This is all about XLOG_SEG_SIZE; I presume XLOG_BLCKSZ can stay as it
> is, right?)

Avoiding variable sizes does avoid some failure modes on the
filesystem side in the face of crashes/power loss.

So making them variable size, while possible, wouldn't be simple at
all (it would involve figuring out the way filesystems behave when
facing crash when a file is changing in size).

On Thu, Aug 25, 2016 at 11:21 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 25 August 2016 at 02:31, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Furthermore, there is an enforced, synchronous fsync at the end of
>> every segment, which actually does hurt performance on write-heavy
>> workloads.[2] Of course, if that were the only reason to consider
>> increasing the segment size, it would probably make more sense to just
>> try to push that extra fsync into the background, but that's not
>> really the case. From what I hear, the gigantic number of files is a
>> bigger pain point.
>
> I think we should fully describe the problem before finding a solution.

Sure, that's usually a good idea. I attempted to outline all of the
possible issues of which I am aware in my original email, but of
course you may know of considerations which I overlooked.

> This is too big a change to just tweak a value without discussing the
> actual issue.

Again, I tried to make sure I was discussing the actual issues in my
original email. In brief: having to run archive_command multiple
times per second imposes very tight latency requirements on it;
directories with hundreds of thousands or millions of files are hard
to manage; enforced synchronous fsyncs at the end of each segment hurt
performance.

> And if the problem is as described, how can a change of x4 be enough
> to make it worth the pain of change? I think you're already admitting
> it can't be worth it by discussing initdb configuration.

I guess it depends on how much pain of change you think there will be.
I would expect a change from 16MB -> 64MB to be fairly painless, but
(1) it might break tools that aren't designed to cope with differing
segment sizes and (2) it will increase disk utilization for people who
have such low velocity systems that they never end up with more than 2
WAL segments, and now those segments are bigger. If you know of other
impacts or have reason to believe those problems will be serious,
please fill in the details.

Despite the fact that initdb configuration has dominated this thread,
I mentioned it only in the very last sentence of my email and only as
a possibility. I believe that a 4x change will be good enough for the
majority of people for whom this is currently a pain point. However,
yes, I do believe that there are some people for whom it won't be
sufficient. And I believe that as we continue to enhance PostgreSQL
to support higher and higher transaction rates, the number of people
who need an extra-large WAL segment size will increase. As I see it,
there are three options here:

1. Do nothing. So far, I don't see anybody arguing for that.

2. Change the default to 64MB and call it good. This idea seems to
have considerable support.

3. Allow initdb-time configurability but keep the default at 16MB. I
don't see any support for this. There is clearly support for
configurability, but I don't see anyone arguing that the current
default is preferable, unless that is what you are arguing.

4. Change the default to 64MB and also allow initdb-time
configurability. This option also appears to enjoy substantial
support, perhaps more than #2. Magnus seemed to be arguing that this
is preferable to #2, because then it's easier for people to change the
setting back if someone discovers a case where the higher default is a
problem; Tom, on the other hand, seems to think this is overkill.

Personally, I believe option #4 is for the best. I believe that the
great majority of users will be better off with 64MB than with 16MB,
but I like the idea of allowing for smaller values (for people with
really low-velocity instances) and larger ones (for people with really
high-velocity instances).

> If we do have the pain of change, should we also consider making WAL
> files variable length? What do we gain by having the files all the
> same size? ISTM better to have WAL files that vary in length up to 1GB
> in size.

This seems like an odd comment because the whole way we address WAL
positions is based on the fact that segments are fixed size, as I
would have thought you would know better than I. The file that
contains a particular byte of WAL is based on lsn/XLOG_SEG_SIZE and
the position within the file is lsn%XLOG_SEG_SIZE. Making files
variable-size would vastly complicate this addressing scheme and maybe
hurt performance in the process. I can't see any compelling reason to
go there.

> (This is all about XLOG_SEG_SIZE; I presume XLOG_BLCKSZ can stay as it
> is, right?)

Yep. Or at least, any discussion of changing the default XLOG block
size would be a completely separate from the issues raised here.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Aug 25, 2016 at 6:59 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Aug 25, 2016 at 11:21 AM, Simon Riggs <simon(at)2ndquadrant(dot)com>
> wrote:
> > On 25 August 2016 at 02:31, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >> Furthermore, there is an enforced, synchronous fsync at the end of
> >> every segment, which actually does hurt performance on write-heavy
> >> workloads.[2] Of course, if that were the only reason to consider
> >> increasing the segment size, it would probably make more sense to just
> >> try to push that extra fsync into the background, but that's not
> >> really the case. From what I hear, the gigantic number of files is a
> >> bigger pain point.
> >
> > I think we should fully describe the problem before finding a solution.
>
> Sure, that's usually a good idea. I attempted to outline all of the
> possible issues of which I am aware in my original email, but of
> course you may know of considerations which I overlooked.
>
> > This is too big a change to just tweak a value without discussing the
> > actual issue.
>
> Again, I tried to make sure I was discussing the actual issues in my
> original email. In brief: having to run archive_command multiple
> times per second imposes very tight latency requirements on it;
> directories with hundreds of thousands or millions of files are hard
> to manage; enforced synchronous fsyncs at the end of each segment hurt
> performance.
>
> > And if the problem is as described, how can a change of x4 be enough
> > to make it worth the pain of change? I think you're already admitting
> > it can't be worth it by discussing initdb configuration.
>
> I guess it depends on how much pain of change you think there will be.
> I would expect a change from 16MB -> 64MB to be fairly painless, but
> (1) it might break tools that aren't designed to cope with differing
> segment sizes and (2) it will increase disk utilization for people who
> have such low velocity systems that they never end up with more than 2
> WAL segments, and now those segments are bigger. If you know of other
> impacts or have reason to believe those problems will be serious,
> please fill in the details.
>
> Despite the fact that initdb configuration has dominated this thread,
> I mentioned it only in the very last sentence of my email and only as
> a possibility. I believe that a 4x change will be good enough for the
> majority of people for whom this is currently a pain point. However,
> yes, I do believe that there are some people for whom it won't be
> sufficient. And I believe that as we continue to enhance PostgreSQL
> to support higher and higher transaction rates, the number of people
> who need an extra-large WAL segment size will increase. As I see it,
> there are three options here:
>
> 1. Do nothing. So far, I don't see anybody arguing for that.
>
> 2. Change the default to 64MB and call it good. This idea seems to
> have considerable support.
>
> 3. Allow initdb-time configurability but keep the default at 16MB. I
> don't see any support for this. There is clearly support for
> configurability, but I don't see anyone arguing that the current
> default is preferable, unless that is what you are arguing.
>
> 4. Change the default to 64MB and also allow initdb-time
> configurability. This option also appears to enjoy substantial
> support, perhaps more than #2. Magnus seemed to be arguing that this
> is preferable to #2, because then it's easier for people to change the
> setting back if someone discovers a case where the higher default is a
> problem; Tom, on the other hand, seems to think this is overkill.

> Personally, I believe option #4 is for the best. I believe that the
> great majority of users will be better off with 64MB than with 16MB,
> but I like the idea of allowing for smaller values (for people with
> really low-velocity instances) and larger ones (for people with really
> high-velocity instances).
>

I was not arguing for #4 over #2, at least not strongly. I think #2 is
fine, and I think #4 are fine. #4 allows a way out, but it's not *that*
important unless we go *beyond* 64Mb.

I was mainly arguing that we can't claim "it has a configure switch so it's
kinda configurable" as a way out. If we want it configurable *at all*, it
should be an initdb switch. If we are confident in our defaults, it doesn't
have to be.

I agree that #4 is best. I'm not sure it's worth the cost. I'm not worried
at all about the risk of master/slave sync thing, per previous statement.
But if it does have performance implications, per Andres suggestion, then
making it configurable at initdb time probably comes with a cost that's not
worth paying.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Thu, Aug 25, 2016 at 1:05 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> 1. Do nothing. So far, I don't see anybody arguing for that.
>>
>> 2. Change the default to 64MB and call it good. This idea seems to
>> have considerable support.
>>
>> 3. Allow initdb-time configurability but keep the default at 16MB. I
>> don't see any support for this. There is clearly support for
>> configurability, but I don't see anyone arguing that the current
>> default is preferable, unless that is what you are arguing.
>>
>> 4. Change the default to 64MB and also allow initdb-time
>> configurability. This option also appears to enjoy substantial
>> support, perhaps more than #2. Magnus seemed to be arguing that this
>> is preferable to #2, because then it's easier for people to change the
>> setting back if someone discovers a case where the higher default is a
>> problem; Tom, on the other hand, seems to think this is overkill.
>
> I was not arguing for #4 over #2, at least not strongly. I think #2 is fine,
> and I think #4 are fine. #4 allows a way out, but it's not *that* important
> unless we go *beyond* 64Mb.

OK, thanks for clarifying. I can't see going beyond 64MB by default
when we're shipping max_wal_size=1GB. In another 20 years when
PB-size thumb drives are commonplace we might reconsider.

> I was mainly arguing that we can't claim "it has a configure switch so it's
> kinda configurable" as a way out. If we want it configurable *at all*, it
> should be an initdb switch. If we are confident in our defaults, it doesn't
> have to be.
>
> I agree that #4 is best. I'm not sure it's worth the cost. I'm not worried
> at all about the risk of master/slave sync thing, per previous statement.
> But if it does have performance implications, per Andres suggestion, then
> making it configurable at initdb time probably comes with a cost that's not
> worth paying.

At this point it's hard to judge, because we don't have any idea what
the cost might be. I guess if we want to pursue this approach,
somebody will have to code it up and benchmark it. But what I'm
inclined to do for starters is put together a patch to go from 16MB ->
64MB. Committing that early this cycle will give us time to
reconsider if that turns out to be painful for reasons we haven't
thought of yet. And give tool authors time to make adjustments, if
any are needed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 08/25/2016 01:12 PM, Robert Haas wrote:
>> I agree that #4 is best. I'm not sure it's worth the cost. I'm not worried
>> > at all about the risk of master/slave sync thing, per previous statement.
>> > But if it does have performance implications, per Andres suggestion, then
>> > making it configurable at initdb time probably comes with a cost that's not
>> > worth paying.
> At this point it's hard to judge, because we don't have any idea what
> the cost might be. I guess if we want to pursue this approach,
> somebody will have to code it up and benchmark it. But what I'm
> inclined to do for starters is put together a patch to go from 16MB ->
> 64MB. Committing that early this cycle will give us time to
> reconsider if that turns out to be painful for reasons we haven't
> thought of yet. And give tool authors time to make adjustments, if
> any are needed.

The one thing I'd be worried about with the increase in size is folks
using PostgreSQL for very small databases. If your database is only
30MB or so in size, the increase in size of the WAL will be pretty
significant (+144MB for the base 3 WAL segments). I'm not sure this is
a real problem which users will notice (in today's scales, 144MB ain't
much), but if it turns out to be, it would be nice to have a way to
switch it back *just for them* without recompiling.

--
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

On Thu, Aug 25, 2016 at 1:43 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 08/25/2016 01:12 PM, Robert Haas wrote:
>>> I agree that #4 is best. I'm not sure it's worth the cost. I'm not worried
>>> > at all about the risk of master/slave sync thing, per previous statement.
>>> > But if it does have performance implications, per Andres suggestion, then
>>> > making it configurable at initdb time probably comes with a cost that's not
>>> > worth paying.
>> At this point it's hard to judge, because we don't have any idea what
>> the cost might be. I guess if we want to pursue this approach,
>> somebody will have to code it up and benchmark it. But what I'm
>> inclined to do for starters is put together a patch to go from 16MB ->
>> 64MB. Committing that early this cycle will give us time to
>> reconsider if that turns out to be painful for reasons we haven't
>> thought of yet. And give tool authors time to make adjustments, if
>> any are needed.
>
> The one thing I'd be worried about with the increase in size is folks
> using PostgreSQL for very small databases. If your database is only
> 30MB or so in size, the increase in size of the WAL will be pretty
> significant (+144MB for the base 3 WAL segments). I'm not sure this is
> a real problem which users will notice (in today's scales, 144MB ain't
> much), but if it turns out to be, it would be nice to have a way to
> switch it back *just for them* without recompiling.

I think you may be forgetting that "the base 3 WAL segments" is no
longer the default configuration. checkpoint_segments=3 is history;
we now have max_wal_size=1GB, which is a maximum of 64 WAL segments,
not 3.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Aug 25, 2016 at 7:45 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Aug 25, 2016 at 1:43 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> > On 08/25/2016 01:12 PM, Robert Haas wrote:
> >>> I agree that #4 is best. I'm not sure it's worth the cost. I'm not
> worried
> >>> > at all about the risk of master/slave sync thing, per previous
> statement.
> >>> > But if it does have performance implications, per Andres suggestion,
> then
> >>> > making it configurable at initdb time probably comes with a cost
> that's not
> >>> > worth paying.
> >> At this point it's hard to judge, because we don't have any idea what
> >> the cost might be. I guess if we want to pursue this approach,
> >> somebody will have to code it up and benchmark it. But what I'm
> >> inclined to do for starters is put together a patch to go from 16MB ->
> >> 64MB. Committing that early this cycle will give us time to
> >> reconsider if that turns out to be painful for reasons we haven't
> >> thought of yet. And give tool authors time to make adjustments, if
> >> any are needed.
> >
> > The one thing I'd be worried about with the increase in size is folks
> > using PostgreSQL for very small databases. If your database is only
> > 30MB or so in size, the increase in size of the WAL will be pretty
> > significant (+144MB for the base 3 WAL segments). I'm not sure this is
> > a real problem which users will notice (in today's scales, 144MB ain't
> > much), but if it turns out to be, it would be nice to have a way to
> > switch it back *just for them* without recompiling.
>
> I think you may be forgetting that "the base 3 WAL segments" is no
> longer the default configuration. checkpoint_segments=3 is history;
> we now have max_wal_size=1GB, which is a maximum of 64 WAL segments,
> not 3.
>
>
And obviously that'd be 16 files if we increase the wal segment size. So
the actual maximum size doesn't change, except you can currently set
max_wal_size to something lower than 64Mb. If we change, the minimum value
would become 64Mb, I assume.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On 2016-08-25 13:45:29 -0400, Robert Haas wrote:
> I think you may be forgetting that "the base 3 WAL segments" is no
> longer the default configuration. checkpoint_segments=3 is history;
> we now have max_wal_size=1GB, which is a maximum of 64 WAL segments,
> not 3.

Well, but min_wal_size still is 48MB. So sure, if you consistently have
a high WAL throughput, it'll be bigger. But otherwise pg_xlog will
shrink again.

On Thu, Aug 25, 2016 at 1:50 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-08-25 13:45:29 -0400, Robert Haas wrote:
>> I think you may be forgetting that "the base 3 WAL segments" is no
>> longer the default configuration. checkpoint_segments=3 is history;
>> we now have max_wal_size=1GB, which is a maximum of 64 WAL segments,
>> not 3.
>
> Well, but min_wal_size still is 48MB. So sure, if you consistently have
> a high WAL throughput, it'll be bigger. But otherwise pg_xlog will
> shrink again.

Hmm, yeah.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Thu, Aug 25, 2016 at 1:43 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:

> > The one thing I'd be worried about with the increase in size is folks
> > using PostgreSQL for very small databases. If your database is only
> > 30MB or so in size, the increase in size of the WAL will be pretty
> > significant (+144MB for the base 3 WAL segments). I'm not sure this is
> > a real problem which users will notice (in today's scales, 144MB ain't
> > much), but if it turns out to be, it would be nice to have a way to
> > switch it back *just for them* without recompiling.
>
> I think you may be forgetting that "the base 3 WAL segments" is no
> longer the default configuration. checkpoint_segments=3 is history;
> we now have max_wal_size=1GB, which is a maximum of 64 WAL segments,
> not 3.

I think the relevant one for that case is the minimum, though:

#min_wal_size = 80MB

which corresponds to 5 segments. I suppose the default value for this
minimum would change to some multiple of 64MB.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Aug 25, 2016 at 2:49 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Robert Haas wrote:
>> On Thu, Aug 25, 2016 at 1:43 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>
>> > The one thing I'd be worried about with the increase in size is folks
>> > using PostgreSQL for very small databases. If your database is only
>> > 30MB or so in size, the increase in size of the WAL will be pretty
>> > significant (+144MB for the base 3 WAL segments). I'm not sure this is
>> > a real problem which users will notice (in today's scales, 144MB ain't
>> > much), but if it turns out to be, it would be nice to have a way to
>> > switch it back *just for them* without recompiling.
>>
>> I think you may be forgetting that "the base 3 WAL segments" is no
>> longer the default configuration. checkpoint_segments=3 is history;
>> we now have max_wal_size=1GB, which is a maximum of 64 WAL segments,
>> not 3.
>
> I think the relevant one for that case is the minimum, though:
>
> #min_wal_size = 80MB
>
> which corresponds to 5 segments. I suppose the default value for this
> minimum would change to some multiple of 64MB.

Yeah, Andres made the same point, although it looks like he
erroneously stated that the minimum was 48MB whereas you have it as
80MB, which seems to be the actual value. I assume we would have to
raise that to either 128MB or 192MB, which does feel like a bit of a
hefty increase. It doesn't matter if you're going to make extensive
use of the cluster, but if somebody's spinning up hundreds of clusters
each of which has very little activity it might not be an altogether
welcome change.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Thu, Aug 25, 2016 at 2:49 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:

> > I think the relevant one for that case is the minimum, though:
> >
> > #min_wal_size = 80MB
> >
> > which corresponds to 5 segments. I suppose the default value for this
> > minimum would change to some multiple of 64MB.
>
> Yeah, Andres made the same point, although it looks like he
> erroneously stated that the minimum was 48MB whereas you have it as
> 80MB, which seems to be the actual value. I assume we would have to
> raise that to either 128MB or 192MB, which does feel like a bit of a
> hefty increase. It doesn't matter if you're going to make extensive
> use of the cluster, but if somebody's spinning up hundreds of clusters
> each of which has very little activity it might not be an altogether
> welcome change.

Yeah, and it's also related to the point Josh Berkus was making about
clusters with little activity.

Does it work to set the minimum to one WAL segment, i.e. 64MB? guc.c
has a hardcoded minimum of 2, but I couldn't find an explanation for it.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Aug 25, 2016 at 04:21:33PM +0100, Simon Riggs wrote:
> If we do have the pain of change, should we also consider making WAL
> files variable length? What do we gain by having the files all the
> same size? ISTM better to have WAL files that vary in length up to 1GB
> in size.
>
> (This is all about XLOG_SEG_SIZE; I presume XLOG_BLCKSZ can stay as it
> is, right?)

I think having WAL use variable length files would add complexity for
recycling WAL files.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On Thu, Aug 25, 2016 at 3:21 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Yeah, and it's also related to the point Josh Berkus was making about
> clusters with little activity.

Right.

> Does it work to set the minimum to one WAL segment, i.e. 64MB? guc.c
> has a hardcoded minimum of 2, but I couldn't find an explanation for it.

Well, I think that when you overrun the end of one segment, you're
never going to be able to wrap around to the start of the same
segment; you're going to get sucked into needing another file.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Thu, Aug 25, 2016 at 3:21 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:

> > Does it work to set the minimum to one WAL segment, i.e. 64MB? guc.c
> > has a hardcoded minimum of 2, but I couldn't find an explanation for it.
>
> Well, I think that when you overrun the end of one segment, you're
> never going to be able to wrap around to the start of the same
> segment; you're going to get sucked into needing another file.

Sure, but that's a transient situation; after a couple of checkpoints,
the old segment can be removed without any danger, leaving only the
active segment. [thinks] Ah, on reflection, there's no way that this
buys anything: it is always critical to have enough disk space to have
one more segment to switch to. So even if you're on tight disk
constraints, you cannot afford to allocate space for a single segment
only, because if you only have that and the need comes to create the
next one to switch to, you will just not have the space.

If we were to use the WAL space in a way different than the POSIX
file interface, we could probably do better. But that seems too
onerous.

I suppose the only option is to keep the minimum at 2. I don't see any
point in forcing the minimum to be more than that, however.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Aug 24, 2016 at 6:31 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 3. archive_timeout is no longer a frequently used option. Obviously,
> if you are frequently archiving partial segments, you don't want the
> segment size to be too large, because if it is, each forced segment
> switch potentially wastes a large amount of space (and bandwidth).
> But given streaming replication and pg_receivexlog, the use case for
> archiving partial segments is, at least according to my understanding,
> a lot narrower than it used to be. So, I think we don't have to worry
> as much about keeping forced segment switches cheap as we did during
> the 8.x series.

Heroku uses archive_timeout. It is considered important, because S3
archives are more reliable than EBS storage. We want to cap how much
time can pass before WAL is shipped to S3, to some degree. It's weird
to talk about degrees of durability, since we tend to assume that it's
either/or, but distinctions like that start to matter when you have an
enormous number of databases. S3 has an extremely good track record,
reliability-wise.

We're not too concerned about the overhead of all of this, I think,
because WAL segments consist of zeroes at the end when archive_timeout
is applied (at least from 9.4 on). We compress the WAL segments, and
many zeroes compress very well.

I admit that I haven't looked at it in much detail, but that is my
current understanding.

--
Peter Geoghegan

On 26/08/16 05:43, Josh Berkus wrote:
> On 08/25/2016 01:12 PM, Robert Haas wrote:
>>> I agree that #4 is best. I'm not sure it's worth the cost. I'm not worried
>>>> at all about the risk of master/slave sync thing, per previous statement.
>>>> But if it does have performance implications, per Andres suggestion, then
>>>> making it configurable at initdb time probably comes with a cost that's not
>>>> worth paying.
>> At this point it's hard to judge, because we don't have any idea what
>> the cost might be. I guess if we want to pursue this approach,
>> somebody will have to code it up and benchmark it. But what I'm
>> inclined to do for starters is put together a patch to go from 16MB ->
>> 64MB. Committing that early this cycle will give us time to
>> reconsider if that turns out to be painful for reasons we haven't
>> thought of yet. And give tool authors time to make adjustments, if
>> any are needed.
> The one thing I'd be worried about with the increase in size is folks
> using PostgreSQL for very small databases. If your database is only
> 30MB or so in size, the increase in size of the WAL will be pretty
> significant (+144MB for the base 3 WAL segments). I'm not sure this is
> a real problem which users will notice (in today's scales, 144MB ain't
> much), but if it turns out to be, it would be nice to have a way to
> switch it back *just for them* without recompiling.
>
Let such folk use Microsoft Access??? <Ducks & runs away very fast!>

More seriously:
Surely most such people would be using very old hardware & not likely to
be upgrading to the most recent version of pg in the near future? And
for the ones using modern hardware: either they have enough resources
not to notice, or very probably will know enough to hunt round for a way
to reduce the WAL size - I strongly suspect.

Currently, I'm not support pg in any production environment, and using
it for testing & keeping up-to-date with pg. So it would affect me -
however, I have enough resources so it is no problem in practice.

Cheers,
Gavin

On Thu, Aug 25, 2016 at 10:25 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Wed, Aug 24, 2016 at 10:40:06PM -0300, Claudio Freire wrote:
>> > time instead of requiring a recompile; we probably don't save any
>> > significant number of cycles by compiling this into the server.
>>
>> FWIW, +1
>>
>> We're already hurt by the small segments due to a similar phenomenon
>> as the ssh case: TCP slow start. Designing the archive/recovery
>> command to work around TCP slow start is quite complex, and bigger
>> segments would just be a better thing.
>>
>> Not to mention that bigger segments compress better.
>
> This would be good time to rename pg_xlog and pg_clog directories too.

That would be an excellent timing to do so. The first CF is close by,
and such a change would be better at the beginning of the development
cycle.
--
Michael

Michael,

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> On Thu, Aug 25, 2016 at 10:25 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > On Wed, Aug 24, 2016 at 10:40:06PM -0300, Claudio Freire wrote:
> >> > time instead of requiring a recompile; we probably don't save any
> >> > significant number of cycles by compiling this into the server.
> >>
> >> FWIW, +1
> >>
> >> We're already hurt by the small segments due to a similar phenomenon
> >> as the ssh case: TCP slow start. Designing the archive/recovery
> >> command to work around TCP slow start is quite complex, and bigger
> >> segments would just be a better thing.
> >>
> >> Not to mention that bigger segments compress better.
> >
> > This would be good time to rename pg_xlog and pg_clog directories too.
>
> That would be an excellent timing to do so. The first CF is close by,
> and such a change would be better at the beginning of the development
> cycle.

If we're going to be renaming things, we might wish to consider further
changes, such as putting everything that's temporary & not WAL-logged
into "pgsql_tmp" directories, so we don't need lists of "directories to
exclude" in things like the pg_basebackup-related code.

We should really have an independent thread about this though, as it's
not what Robert's asking about here.

Thanks!

Stephen

On Thu, Aug 25, 2016 at 10:29 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Aug 25, 2016 at 11:21 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On 25 August 2016 at 02:31, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> Furthermore, there is an enforced, synchronous fsync at the end of
>>> every segment, which actually does hurt performance on write-heavy
>>> workloads.[2] Of course, if that were the only reason to consider
>>> increasing the segment size, it would probably make more sense to just
>>> try to push that extra fsync into the background, but that's not
>>> really the case. From what I hear, the gigantic number of files is a
>>> bigger pain point.
>>
>> I think we should fully describe the problem before finding a solution.
>
> Sure, that's usually a good idea. I attempted to outline all of the
> possible issues of which I am aware in my original email, but of
> course you may know of considerations which I overlooked.
>
>> This is too big a change to just tweak a value without discussing the
>> actual issue.
>
> Again, I tried to make sure I was discussing the actual issues in my
> original email. In brief: having to run archive_command multiple
> times per second imposes very tight latency requirements on it;
> directories with hundreds of thousands or millions of files are hard
> to manage; enforced synchronous fsyncs at the end of each segment hurt
> performance.
>
>> And if the problem is as described, how can a change of x4 be enough
>> to make it worth the pain of change? I think you're already admitting
>> it can't be worth it by discussing initdb configuration.
>
> I guess it depends on how much pain of change you think there will be.
> I would expect a change from 16MB -> 64MB to be fairly painless, but
> (1) it might break tools that aren't designed to cope with differing
> segment sizes and (2) it will increase disk utilization for people who
> have such low velocity systems that they never end up with more than 2
> WAL segments, and now those segments are bigger. If you know of other
> impacts or have reason to believe those problems will be serious,
> please fill in the details.
>
> Despite the fact that initdb configuration has dominated this thread,
> I mentioned it only in the very last sentence of my email and only as
> a possibility. I believe that a 4x change will be good enough for the
> majority of people for whom this is currently a pain point. However,
> yes, I do believe that there are some people for whom it won't be
> sufficient. And I believe that as we continue to enhance PostgreSQL
> to support higher and higher transaction rates, the number of people
> who need an extra-large WAL segment size will increase. As I see it,
> there are three options here:
>
> 1. Do nothing. So far, I don't see anybody arguing for that.
>
> 2. Change the default to 64MB and call it good. This idea seems to
> have considerable support.
>
> 3. Allow initdb-time configurability but keep the default at 16MB. I
> don't see any support for this. There is clearly support for
> configurability, but I don't see anyone arguing that the current
> default is preferable, unless that is what you are arguing.
>
> 4. Change the default to 64MB and also allow initdb-time
> configurability. This option also appears to enjoy substantial
> support, perhaps more than #2. Magnus seemed to be arguing that this
> is preferable to #2, because then it's easier for people to change the
> setting back if someone discovers a case where the higher default is a
> problem; Tom, on the other hand, seems to think this is overkill.
>

If we change the default to 64MB, then I think it won't allow to use
old databases as-is because we store it in pg_control (I think one
will get below error [1] for old databases, if we just change default
and don't do anything else). Do you have way to address it or you
think it is okay?

[1] -
FATAL: database files are incompatible with server
DETAIL: The database cluster was initialized with XLOG_SEG_SIZE
16777216, but the server was compiled with XLOG_SEG_SIZE 67108864.
HINT: It looks like you need to recompile or initdb.
LOG: database system is shut down

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Aug 26, 2016 at 12:25 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> If we change the default to 64MB, then I think it won't allow to use
> old databases as-is because we store it in pg_control (I think one
> will get below error [1] for old databases, if we just change default
> and don't do anything else). Do you have way to address it or you
> think it is okay?

Those would still be able to work with ./configure
--with-wal-segsize=16, so that's not really an issue.
--
Michael

Gavin Flower wrote:
> On 26/08/16 05:43, Josh Berkus wrote:

> >The one thing I'd be worried about with the increase in size is folks
> >using PostgreSQL for very small databases. If your database is only
> >30MB or so in size, the increase in size of the WAL will be pretty
> >significant (+144MB for the base 3 WAL segments). I'm not sure this is
> >a real problem which users will notice (in today's scales, 144MB ain't
> >much), but if it turns out to be, it would be nice to have a way to
> >switch it back *just for them* without recompiling.
> >
> Let such folk use Microsoft Access??? <Ducks & runs away very fast!>
>
>
> More seriously:
> Surely most such people would be using very old hardware & not likely to be
> upgrading to the most recent version of pg in the near future? And for the
> ones using modern hardware: either they have enough resources not to notice,
> or very probably will know enough to hunt round for a way to reduce the WAL
> size - I strongly suspect.

I've seen people with unusual environments, such as running Pg in some
embedded platform with minimal resources, where they were baffled that
Postgres used so much disk space on files that were barely written to
and never read. It wasn't a question of there being "large" drives to
buy, but one of not wanting to have a drive in the first place. Now, I
grant that this was a few years ago already and disk tech (SSDs) has
changed that world; maybe that argument doesn't apply anymore.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Aug 26, 2016 at 9:04 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Aug 26, 2016 at 12:25 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> If we change the default to 64MB, then I think it won't allow to use
>> old databases as-is because we store it in pg_control (I think one
>> will get below error [1] for old databases, if we just change default
>> and don't do anything else). Do you have way to address it or you
>> think it is okay?
>
> Those would still be able to work with ./configure
> --with-wal-segsize=16, so that's not really an issue.
>

Right, but do we need suggest users to do so? The question/point was
if we deliver server with default value as 64MB, then it won't allow
to start old database.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Aug 26, 2016 at 12:54 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Aug 26, 2016 at 9:04 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Fri, Aug 26, 2016 at 12:25 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> If we change the default to 64MB, then I think it won't allow to use
>>> old databases as-is because we store it in pg_control (I think one
>>> will get below error [1] for old databases, if we just change default
>>> and don't do anything else). Do you have way to address it or you
>>> think it is okay?
>>
>> Those would still be able to work with ./configure
>> --with-wal-segsize=16, so that's not really an issue.
>>
>
> Right, but do we need suggest users to do so? The question/point was
> if we deliver server with default value as 64MB, then it won't allow
> to start old database.

Right, pg_upgrade could be made smarter by enforcing a conversion with
a dedicated option: we could get away by filling the existing segments
with zeros and add an XLOG switch record at the end of each segments
formerly at 16MB converted to 64MB. That would still be better than
converting each page LSN :(
--
Michael

On 2016-08-26 13:07:09 +0900, Michael Paquier wrote:
> On Fri, Aug 26, 2016 at 12:54 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Fri, Aug 26, 2016 at 9:04 AM, Michael Paquier
> > <michael(dot)paquier(at)gmail(dot)com> wrote:
> >> On Fri, Aug 26, 2016 at 12:25 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >>> If we change the default to 64MB, then I think it won't allow to use
> >>> old databases as-is because we store it in pg_control (I think one
> >>> will get below error [1] for old databases, if we just change default
> >>> and don't do anything else). Do you have way to address it or you
> >>> think it is okay?
> >>
> >> Those would still be able to work with ./configure
> >> --with-wal-segsize=16, so that's not really an issue.
> >>
> >
> > Right, but do we need suggest users to do so? The question/point was
> > if we deliver server with default value as 64MB, then it won't allow
> > to start old database.
>
> Right, pg_upgrade could be made smarter by enforcing a conversion with
> a dedicated option: we could get away by filling the existing segments
> with zeros and add an XLOG switch record at the end of each segments
> formerly at 16MB converted to 64MB. That would still be better than
> converting each page LSN :(

Maybe I'm missing something here - but why would we need to do any of
that? The WAL already isn't compatible between versions, and we don't
reuse the old server's WAL anyway? Isn't all that's needed relaxing some
error check?

On Wed, 24 Aug 2016 21:31:35 -0400
Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> Hi,
>
> I'd like to propose that we increase the default WAL segment size,
> which is currently 16MB. It was first set to that value in commit
> 47937403676d913c0e740eec6b85113865c6c8ab in October of 1999; prior to
> that, it was 64MB. Between 1999 and now, there have been three
> significant changes that make me think it might be time to rethink
> this value:
<snip>
>
> Thoughts?

From my ignorance, could block size affect this WAL size increase?

In past (didn't tried with >9 versions) you can change disk block page size from 8KB to 16 KB or 32KB (or 4) modifing src/include/pg_config.h BLCKSZ 8192 and recompiling. (There are some mails in 1999-2002 about this topic)

> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

--- ---
Eduardo Morras <emorrasg(at)yahoo(dot)es>

On Fri, Aug 26, 2016 at 12:39 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Maybe I'm missing something here - but why would we need to do any of
> that? The WAL already isn't compatible between versions, and we don't
> reuse the old server's WAL anyway? Isn't all that's needed relaxing some
> error check?

Yeah. If this change is made in a new major version - and how else
would we do it? - it doesn't introduce any incompatibility that
wouldn't be present already. pg_upgrade doesn't (and can't) migrate
WAL.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Aug 26, 2016 at 4:45 AM, Eduardo Morras <emorrasg(at)yahoo(dot)es> wrote:
> From my ignorance, could block size affect this WAL size increase?
>
> In past (didn't tried with >9 versions) you can change disk block page size from 8KB to 16 KB or 32KB (or 4) modifing src/include/pg_config.h BLCKSZ 8192 and recompiling. (There are some mails in 1999-2002 about this topic)

Yeah, I think that's still supposed to work although I don't know
whether anyone has tried it lately. It is a separate topic from this
issue, though.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 8/24/16 9:31 PM, Robert Haas wrote:
> I'd like to propose that we increase the default WAL segment size,
> which is currently 16MB.

While the discussion about the best default value is ongoing, maybe we
should at least *allow* some larger sizes, for testing out. Currently,
configure says "Allowed values are 1,2,4,8,16,32,64.". What might be a
good new upper limit?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Sep 20, 2016 at 2:49 PM, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> On 8/24/16 9:31 PM, Robert Haas wrote:
>> I'd like to propose that we increase the default WAL segment size,
>> which is currently 16MB.
>
> While the discussion about the best default value is ongoing, maybe we
> should at least *allow* some larger sizes, for testing out. Currently,
> configure says "Allowed values are 1,2,4,8,16,32,64.". What might be a
> good new upper limit?

1GB?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-09-20 16:05:44 -0400, Robert Haas wrote:
> On Tue, Sep 20, 2016 at 2:49 PM, Peter Eisentraut
> <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> > On 8/24/16 9:31 PM, Robert Haas wrote:
> >> I'd like to propose that we increase the default WAL segment size,
> >> which is currently 16MB.
> >
> > While the discussion about the best default value is ongoing, maybe we
> > should at least *allow* some larger sizes, for testing out. Currently,
> > configure says "Allowed values are 1,2,4,8,16,32,64.". What might be a
> > good new upper limit?

I'm doubtful it's worth increasing this.

> 1GB?

That sounds way too big to me. WAL file allocation would trigger pretty
massive IO storms during zeroing, max_wal_size is going to be hard to
tune, the amounts of dirty data during bulk loads is going to be very
hard to control. If somebody wants to do something like this they
better be well informed enough to override a #define.

Andres

On Tue, Sep 20, 2016 at 4:09 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-09-20 16:05:44 -0400, Robert Haas wrote:
>> On Tue, Sep 20, 2016 at 2:49 PM, Peter Eisentraut
>> <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>> > On 8/24/16 9:31 PM, Robert Haas wrote:
>> >> I'd like to propose that we increase the default WAL segment size,
>> >> which is currently 16MB.
>> >
>> > While the discussion about the best default value is ongoing, maybe we
>> > should at least *allow* some larger sizes, for testing out. Currently,
>> > configure says "Allowed values are 1,2,4,8,16,32,64.". What might be a
>> > good new upper limit?
>
> I'm doubtful it's worth increasing this.
>
>> 1GB?
>
> That sounds way too big to me. WAL file allocation would trigger pretty
> massive IO storms during zeroing, max_wal_size is going to be hard to
> tune, the amounts of dirty data during bulk loads is going to be very
> hard to control. If somebody wants to do something like this they
> better be well informed enough to override a #define.

EnterpriseDB has customers generating multiple TB of WAL per day.
Even with a 1GB segment size, some of them will fill multiple files
per minute. At the current limit of 64MB, a few of them would still
fill more than one file per second. That is not sane.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-09-20 16:18:02 -0400, Robert Haas wrote:
> On Tue, Sep 20, 2016 at 4:09 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > That sounds way too big to me. WAL file allocation would trigger pretty
> > massive IO storms during zeroing, max_wal_size is going to be hard to
> > tune, the amounts of dirty data during bulk loads is going to be very
> > hard to control. If somebody wants to do something like this they
> > better be well informed enough to override a #define.
>
> EnterpriseDB has customers generating multiple TB of WAL per day.

Sure, that's kind of common.

> Even with a 1GB segment size, some of them will fill multiple files
> per minute. At the current limit of 64MB, a few of them would still
> fill more than one file per second. That is not sane.

I doubt generating much larger files actually helps a lot there. I bet
you a patch review that 1GB files are going to regress in pretty much
every situation; especially when taking latency into account.
I think what's actually needed for that is:
- make it easier to implement archiving via streaming WAL; i.e. make
pg_receivexlog actually usable
- make archiving parallel
- decouple WAL write & fsyncing granularity from segment size

Requiring a non-default compile time or even just cluster creation time
option for tuning isn't something worth expanding energy on imo.

Andres

On Tue, Sep 20, 2016 at 4:25 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> Even with a 1GB segment size, some of them will fill multiple files
>> per minute. At the current limit of 64MB, a few of them would still
>> fill more than one file per second. That is not sane.
>
> I doubt generating much larger files actually helps a lot there. I bet
> you a patch review that 1GB files are going to regress in pretty much
> every situation; especially when taking latency into account.

Well, you have a point: let's find out. Suppose we create a cluster
that generates WAL very quickly, and then try different WAL segment
sizes and see what works out best. Maybe something like: create N
relatively small tables, with 100 or so tuples in each one. Have N
backends, each assigned one of those tables, and it just updates all
the rows over and over in a tight loop. Or feel free to suggest
something else.

> I think what's actually needed for that is:
> - make it easier to implement archiving via streaming WAL; i.e. make
> pg_receivexlog actually usable
> - make archiving parallel
> - decouple WAL write & fsyncing granularity from segment size
>
> Requiring a non-default compile time or even just cluster creation time
> option for tuning isn't something worth expanding energy on imo.

I don't agree. The latency requirements on an archive_command when
you're churning out 16MB files multiple times per second are insanely
tight, and saying that we shouldn't increase the size because it's
better to go redesign a bunch of other things that will eventually
*maybe* remove the need for archive_command does not seem like a
reasonable response.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-09-20 16:32:46 -0400, Robert Haas wrote:
> > Requiring a non-default compile time or even just cluster creation time
> > option for tuning isn't something worth expanding energy on imo.
>
> I don't agree. The latency requirements on an archive_command when
> you're churning out 16MB files multiple times per second are insanely
> tight, and saying that we shouldn't increase the size because it's
> better to go redesign a bunch of other things that will eventually
> *maybe* remove the need for archive_command does not seem like a
> reasonable response.

Oh, I'm on board with increasing the default size a bit. A different
default size isn't a non-default compile time option anymore though, and
I don't think 1GB is a reasonable default.

Running multiple archive_commands concurrently - pretty easy to
implement - isn't the same as removing the need for archive command. I'm
pretty sure that continously,and if necessary concurrently, archiving a
bunch of 64MB files is going to work better than irregularly
creating / transferring 1GB files.

Andres

On Tue, Sep 20, 2016 at 4:42 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-09-20 16:32:46 -0400, Robert Haas wrote:
>> > Requiring a non-default compile time or even just cluster creation time
>> > option for tuning isn't something worth expanding energy on imo.
>>
>> I don't agree. The latency requirements on an archive_command when
>> you're churning out 16MB files multiple times per second are insanely
>> tight, and saying that we shouldn't increase the size because it's
>> better to go redesign a bunch of other things that will eventually
>> *maybe* remove the need for archive_command does not seem like a
>> reasonable response.
>
> Oh, I'm on board with increasing the default size a bit. A different
> default size isn't a non-default compile time option anymore though, and
> I don't think 1GB is a reasonable default.

But that's not the question. What Peter said was: "maybe we should at
least *allow* some larger sizes, for testing out". I see very little
merit in restricting the values that people can set via configure.
That just makes life difficult. If a user picks a setting that
doesn't perform well, oops.

> Running multiple archive_commands concurrently - pretty easy to
> implement - isn't the same as removing the need for archive command. I'm
> pretty sure that continously,and if necessary concurrently, archiving a
> bunch of 64MB files is going to work better than irregularly
> creating / transferring 1GB files.

I'm not trying to block you from implementing parallel archiving, but
right now we don't have it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 9/21/16 8:12 AM, Robert Haas wrote:
>> Oh, I'm on board with increasing the default size a bit. A different
>> > default size isn't a non-default compile time option anymore though, and
>> > I don't think 1GB is a reasonable default.
> But that's not the question. What Peter said was: "maybe we should at
> least *allow* some larger sizes, for testing out". I see very little
> merit in restricting the values that people can set via configure.
> That just makes life difficult. If a user picks a setting that
> doesn't perform well, oops.

Right. If we think that a larger size can have some performance benefit
and we think that 64MB might be a good new default (as was the initial
suggestion), then we should surely allow at least say 128 and 256 to be
tried out.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello all,

Please find attached a patch to make wal segment size initdb configurable.

The attached patch removes --with-wal-segsize configure option and adds a
new initdb option --wal-segsize. The module initdb passes the wal-segsize
value into an environment variable which is used to overwrite the guc value
wal_ segment_size and set the internal variables : XLogSegSize and
XLOG_SEG_SIZE (xlog_internal.h). The default wal_segment_size is not
changed but I have increased the maximum size to 1GB.

Since XLOG_SEG_SIZE is now variable, it could not be used directly in
src/bin modules and few macros and few changes had to be made:
- in guc.c , remove GUC_UNIT_XSEGS which used XLOG_SEG_SIZE and
introduce show functions for the guc which used the unit (min_wal_size and
max_wal_size).
- For pg_basebackup, add new replication command SHOW_WAL_SEGSZ to fetch
the wal_segment_size in bytes.
- pg_controldata, pg_resetxlog, pg_rewind, fetch the xlog_seg_size from
the ControlFile.
- Since pg_xlogdump reads the wal files, it uses the file size to
determine the xlog_seg_size.
- In pg_test_fsync, a buffer of size XLOG_SEG_SIZE was created, filled
with random data and written to a temporary file to check for any
write/fsync error before performing the tests. Since it does not affect the
actual performance results, the XLOG_SEG_SIZE in the module is replaced
with the default value (16MB).

Please note that the documents are not updated in this patch.

Feedback and suggestions are welcome.
--

Beena Emerson

Have a Great Day!

Hello all,

On Mon, Dec 19, 2016 at 3:14 PM, Beena Emerson <memissemerson(at)gmail(dot)com>
wrote:

> Hello all,
>
> Please find attached a patch to make wal segment size initdb configurable.
>
> The attached patch removes --with-wal-segsize configure option and adds a
> new initdb option --wal-segsize. The module initdb passes the wal-segsize
> value into an environment variable which is used to overwrite the guc value
> wal_ segment_size and set the internal variables : XLogSegSize and
> XLOG_SEG_SIZE (xlog_internal.h). The default wal_segment_size is not
> changed but I have increased the maximum size to 1GB.
>
> Since XLOG_SEG_SIZE is now variable, it could not be used directly in
> src/bin modules and few macros and few changes had to be made:
> - in guc.c , remove GUC_UNIT_XSEGS which used XLOG_SEG_SIZE and
> introduce show functions for the guc which used the unit (min_wal_size and
> max_wal_size).
> - For pg_basebackup, add new replication command SHOW_WAL_SEGSZ to
> fetch the wal_segment_size in bytes.
> - pg_controldata, pg_resetxlog, pg_rewind, fetch the xlog_seg_size from
> the ControlFile.
> - Since pg_xlogdump reads the wal files, it uses the file size to
> determine the xlog_seg_size.
> - In pg_test_fsync, a buffer of size XLOG_SEG_SIZE was created, filled
> with random data and written to a temporary file to check for any
> write/fsync error before performing the tests. Since it does not affect the
> actual performance results, the XLOG_SEG_SIZE in the module is replaced
> with the default value (16MB).
>
> Please note that the documents are not updated in this patch.
>
> Feedback and suggestions are welcome.
>

This patch has been added to the commit fest (https://commitfest.
postgresql.org/12/921/)

After further testing, I found that pg_standby contrib module does not work
with the changes. I will fix it in the next version of the patch. Comments
on the current patch are welcome.

Thank you,

Beena Emerson

Have a Great Day!

Hi,

On 2016-12-19 15:14:50 +0530, Beena Emerson wrote:
> The attached patch removes --with-wal-segsize configure option and adds a
> new initdb option --wal-segsize. The module initdb passes the wal-segsize
> value into an environment variable which is used to overwrite the guc value
> wal_ segment_size and set the internal variables : XLogSegSize and
> XLOG_SEG_SIZE (xlog_internal.h). The default wal_segment_size is not
> changed but I have increased the maximum size to 1GB.
>
> Since XLOG_SEG_SIZE is now variable, it could not be used directly in
> src/bin modules and few macros and few changes had to be made:

I do think this has the potential for negative performance
implications. Consider code like
/* skip over the page header */
if (CurrPos % XLogSegSize == 0)
{
CurrPos += SizeOfXLogLongPHD;
currpos += SizeOfXLogLongPHD;
}
else
right now that's doable in an efficient manner, because XLogSegSize is
constant. If it's a variable and the compiler doesn't even know it's a
power-of-two, it'll have to do a full "div" - and that's quite easily
noticeable in a lot of cases.

Now it could entirely be that the costs of this will be swamped by
everything else, but I'd not want to rely on it.

I think we need tests with concurrent large-file copies. And then also
look at the profile to see whether the relevant places become new
hotspots (not that we introduce something that's just hidden for now).

We might be able to do a bit better, efficency wise, by storing
XLogSegSize as a "shift factor". I.e. the 16M setting would be 24
(i.e. XLogSegSize would be defined as 1 << 24).

Greetings,

Andres Freund

Hello Andres,

On Tue, Dec 20, 2016 at 1:58 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> Hi,
>
> On 2016-12-19 15:14:50 +0530, Beena Emerson wrote:
> > The attached patch removes --with-wal-segsize configure option and adds a
> > new initdb option --wal-segsize. The module initdb passes the wal-segsize
> > value into an environment variable which is used to overwrite the guc
> value
> > wal_ segment_size and set the internal variables : XLogSegSize and
> > XLOG_SEG_SIZE (xlog_internal.h). The default wal_segment_size is not
> > changed but I have increased the maximum size to 1GB.
> >
> > Since XLOG_SEG_SIZE is now variable, it could not be used directly in
> > src/bin modules and few macros and few changes had to be made:
>
> I do think this has the potential for negative performance
> implications. Consider code like
> /* skip over the page header */
> if (CurrPos % XLogSegSize == 0)
> {
> CurrPos += SizeOfXLogLongPHD;
> currpos += SizeOfXLogLongPHD;
> }
> else
> right now that's doable in an efficient manner, because XLogSegSize is
> constant. If it's a variable and the compiler doesn't even know it's a
> power-of-two, it'll have to do a full "div" - and that's quite easily
> noticeable in a lot of cases.
>
> Now it could entirely be that the costs of this will be swamped by
> everything else, but I'd not want to rely on it.
>
> I think we need tests with concurrent large-file copies. And then also
> look at the profile to see whether the relevant places become new
> hotspots (not that we introduce something that's just hidden for now).

> We might be able to do a bit better, efficency wise, by storing
> XLogSegSize as a "shift factor". I.e. the 16M setting would be 24
> (i.e. XLogSegSize would be defined as 1 << 24).
>
>
Thank you.
I did not realize we could face such problems. I will perform the tests to
check the performance and do the required changes.

--

Beena Emerson

Have a Great Day!

On Tue, Dec 20, 2016 at 3:28 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> I do think this has the potential for negative performance
> implications. Consider code like
> /* skip over the page header */
> if (CurrPos % XLogSegSize == 0)
> {
> CurrPos += SizeOfXLogLongPHD;
> currpos += SizeOfXLogLongPHD;
> }
> else
> right now that's doable in an efficient manner, because XLogSegSize is
> constant. If it's a variable and the compiler doesn't even know it's a
> power-of-two, it'll have to do a full "div" - and that's quite easily
> noticeable in a lot of cases.
>
> Now it could entirely be that the costs of this will be swamped by
> everything else, but I'd not want to rely on it.

We could use the GUC assign hook to compute a mask and a shift, so
that this could be written as (CurrPos & mask_variable) == 0. That
would avoid the division instruction, though not the memory access. I
hope this is all in the noise, though. I know this is code is hot but
I think it'll be hard to construct a test case where the bottleneck is
anything other than the speed at which the disk can absorb bytes. I
suppose we could set fsync=off and put the whole cluster on a RAMDISK
to avoid those bottlenecks, but of course no real storage system
behaves like that.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-12-20 08:10:29 -0500, Robert Haas wrote:
> We could use the GUC assign hook to compute a mask and a shift, so
> that this could be written as (CurrPos & mask_variable) == 0. That
> would avoid the division instruction, though not the memory access.

I suspect that'd be fine.

> I hope this is all in the noise, though.

Could very well be.

> I know this is code is hot but I think it'll be hard to construct a
> test case where the bottleneck is anything other than the speed at
> which the disk can absorb bytes.

I don't think that's really true. Heikki's WAL changes made a *BIG*
difference. And pretty small changes in xlog.c can make noticeable
throughput differences both in single and multi-threaded
workloads. E.g. witnessed by the fact that the crc computation used to
be a major bottleneck (and the crc32c instruction still shows up
noticeably in profiles). SSDs have become fast enough that it's
increasingly hard to saturate them.

Andres

On 12/20/2016 02:19 PM, Andres Freund wrote:
> On 2016-12-20 08:10:29 -0500, Robert Haas wrote:
>> We could use the GUC assign hook to compute a mask and a shift, so
>> that this could be written as (CurrPos & mask_variable) == 0. That
>> would avoid the division instruction, though not the memory access.
>
> I suspect that'd be fine.
>
>
>> I hope this is all in the noise, though.
>
> Could very well be.
>
>
>> I know this is code is hot but I think it'll be hard to construct a
>> test case where the bottleneck is anything other than the speed at
>> which the disk can absorb bytes.
>
> I don't think that's really true. Heikki's WAL changes made a *BIG*
> difference. And pretty small changes in xlog.c can make noticeable
> throughput differences both in single and multi-threaded
> workloads. E.g. witnessed by the fact that the crc computation used to
> be a major bottleneck (and the crc32c instruction still shows up
> noticeably in profiles). SSDs have become fast enough that it's
> increasingly hard to saturate them.
>

It's not just SSDs. RAID controllers with write cache (which is
typically just DRR3 memory anyway) have about the same effect even with
spinning rust.

So yes, this might make a measurable difference.

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

General comments:
There was some discussion about the impact of this on small installs, particularly around min_wal_size. The concern was that changing the default segment size to 64MB would significantly increase min_wal_size in terms of bytes. The default value for min_wal_size is 5 segments, so 16MB->64MB would mean going from 80MB to 320MB. IMHO if you're worried about that then just initdb with a smaller segment size. There's probably a number of other changes a small environment wants to make besides that. Perhaps it'd be worth making DEFAULT_XLOG_SEG_SIZE a configure option to better support that.

It's not clear from the thread that there is consensus that this feature is desired. In particular, the performance aspects of changing segment size from a C constant to a variable are in question. Someone with access to large hardware should test that. Andres[1] and Robert[2] did suggest that the option could be changed to a bitshift, which IMHO would also solve some sanity-checking issues.

+ * initdb passes the WAL segment size in an environment variable. We don't
+ * bother doing any sanity checking, we already check in initdb that the
+ * user gives a sane value.

That doesn't seem like a good idea to me. If anything, the backend should sanity-check and initdb just rely on that. Perhaps this is how other initdb options work, but it still seems bogus. In particular, verifying the size is a power of 2 seems important, as failing that would probably be ReallyBad(tm).

The patch also blindly trusts the value read from the control file; I'm not sure if that's standard procedure or not, but ISTM it'd be worth sanity-checking that as well.

The patch leaves the base GUC units for min_wal_size and max_wal_size as the # of segments. I'm not sure if that's a great idea.

+ * convert_unit
+ *
+ * This takes the value in kbytes and then returns value in user-readable format

This function needs a more specific name, such as pretty_print_kb().

+ /* Check if wal_segment_size is in the power of 2 */
+ for (i = 0;; i++, pow2 = pow(2, i))
+ if (pow2 >= wal_segment_size)
+ break;
+
+ if (wal_segment_size != 1 && pow2 > wal_segment_size)
+ {
+ fprintf(stderr, _("%s: WAL segment size must be in the power of 2\n"), progname);
+ exit(1);
+ }

IMHO it'd be better to use the n & (n-1) check detailed at [3].

Actually, there's got to be other places that need to check this, so it'd be nice to just create a function that verifies a number is a power of 2.

+ if (log_fname != NULL)
+ XLogFromFileName(log_fname, &minXlogTli, &minXlogSegNo);
+

Please add a comment about why XLogFromFileName has to be delayed.

/*
+ * DEFAULT_XLOG_SEG_SIZE is the size of a single WAL file. This must be a power
+ * of 2 and larger than XLOG_BLCKSZ (preferably, a great deal larger than
+ * XLOG_BLCKSZ).
+ *
+ * Changing DEFAULT_XLOG_SEG_SIZE requires an initdb.
+ */
+#define DEFAULT_XLOG_SEG_SIZE (16*1024*1024)

That comment isn't really accurate. It would be more useful to explain that DEFAULT_XLOG_SEG_SIZE is the default size of a WAL segment used by initdb if a different value isn't specified.

1: https://www.postgresql.org/message-id/20161220082847.7t3t6utvxb6m5tfe%40alap3.anarazel.de
2: https://www.postgresql.org/message-id/CA%2BTgmoZTgnL25o68uPBTS6BD37ojD-1y-N88PkP57FzKqwcmmQ%40mail.gmail.com
3: http://stackoverflow.com/questions/108318/whats-the-simplest-way-to-test-whether-a-number-is-a-power-of-2-in-c

On Tue, Jan 3, 2017 at 6:23 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> + /* Check if wal_segment_size is in the power of 2 */
> + for (i = 0;; i++, pow2 = pow(2, i))
> + if (pow2 >= wal_segment_size)
> + break;
> +
> + if (wal_segment_size != 1 && pow2 > wal_segment_size)
> + {
> + fprintf(stderr, _("%s: WAL segment size must be in the power of 2\n"), progname);
> + exit(1);
> + }

I recall taht pow(x, 2) and x * x result usually in the same assembly
code, but pow() can never be more optimal than a simple
multiplication. So I'd think that it is wiser to avoid it in this code
path. Documentation is missing for the new replication command
SHOW_WAL_SEG. Actually, why not just having an equivalent of the SQL
command and be able to query parameter values?
--
Michael

On 2 January 2017 at 21:23, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:

> It's not clear from the thread that there is consensus that this feature is desired. In particular, the performance aspects of changing segment size from a C constant to a variable are in question. Someone with access to large hardware should test that. Andres[1] and Robert[2] did suggest that the option could be changed to a bitshift, which IMHO would also solve some sanity-checking issues.

Overall, Robert has made a good case. The only discussion now is about
the knock-on effects it causes.

One concern that has only barely been discussed is the effect of
zero-ing new WAL files. That is a linear effect and will adversely
effect performance as WAL segment size increases. (The already stated
fsync problem is also a linear effect but that reduces with WAL
segment size, hence the need for a trade-off and hence why
variable-size is preferable).

If we wish this feature to get committed ISTM that we should examine
server performance with a large fixed WAL segment size, so we can
measure the effects of this, particularly with regard to the poor user
that gets to add a new WAL file. ISTM that may reveal more work is
needed to be handed off to the WALWriter process (or other
issues/solutions).

Once we have that information we can consider whether to apply this
patch, so until then, -1 to apply this, though I am hopeful that this
can be applied in this release.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jan 3, 2017 at 6:41 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 2 January 2017 at 21:23, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>
>> It's not clear from the thread that there is consensus that this feature is desired. In particular, the performance aspects of changing segment size from a C constant to a variable are in question. Someone with access to large hardware should test that. Andres[1] and Robert[2] did suggest that the option could be changed to a bitshift, which IMHO would also solve some sanity-checking issues.
>
> Overall, Robert has made a good case. The only discussion now is about
> the knock-on effects it causes.
>
> One concern that has only barely been discussed is the effect of
> zero-ing new WAL files. That is a linear effect and will adversely
> effect performance as WAL segment size increases.
>

Sorry, but I am not able to understand why this is a problem? The
bigger the size of WAL segment, lesser the number of files. So IIUC,
then it can only impact if zero-ing two 16MB files is cheaper than
zero-ing one 32MB file. Is that your theory or you have something
else in mind?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 3 January 2017 at 13:45, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Tue, Jan 3, 2017 at 6:41 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On 2 January 2017 at 21:23, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>>
>>> It's not clear from the thread that there is consensus that this feature is desired. In particular, the performance aspects of changing segment size from a C constant to a variable are in question. Someone with access to large hardware should test that. Andres[1] and Robert[2] did suggest that the option could be changed to a bitshift, which IMHO would also solve some sanity-checking issues.
>>
>> Overall, Robert has made a good case. The only discussion now is about
>> the knock-on effects it causes.
>>
>> One concern that has only barely been discussed is the effect of
>> zero-ing new WAL files. That is a linear effect and will adversely
>> effect performance as WAL segment size increases.
>>
>
> Sorry, but I am not able to understand why this is a problem? The
> bigger the size of WAL segment, lesser the number of files. So IIUC,
> then it can only impact if zero-ing two 16MB files is cheaper than
> zero-ing one 32MB file. Is that your theory or you have something
> else in mind?

The issue I see is that at present no backend needs to do more than
16MB of zeroing at one time, so the impact on response time is
reduced. If we start doing zeroing in larger chunks than the impact on
response times will increase. So instead of regular blips we have one
large blip, less often. I think the latter will be worse, but welcome
measurements that show that performance is smooth and regular with
large files sizes.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jan 3, 2017 at 8:59 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 3 January 2017 at 13:45, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Tue, Jan 3, 2017 at 6:41 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> On 2 January 2017 at 21:23, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>>>
>>>> It's not clear from the thread that there is consensus that this feature is desired. In particular, the performance aspects of changing segment size from a C constant to a variable are in question. Someone with access to large hardware should test that. Andres[1] and Robert[2] did suggest that the option could be changed to a bitshift, which IMHO would also solve some sanity-checking issues.
>>>
>>> Overall, Robert has made a good case. The only discussion now is about
>>> the knock-on effects it causes.
>>>
>>> One concern that has only barely been discussed is the effect of
>>> zero-ing new WAL files. That is a linear effect and will adversely
>>> effect performance as WAL segment size increases.
>>>
>>
>> Sorry, but I am not able to understand why this is a problem? The
>> bigger the size of WAL segment, lesser the number of files. So IIUC,
>> then it can only impact if zero-ing two 16MB files is cheaper than
>> zero-ing one 32MB file. Is that your theory or you have something
>> else in mind?
>
> The issue I see is that at present no backend needs to do more than
> 16MB of zeroing at one time, so the impact on response time is
> reduced. If we start doing zeroing in larger chunks than the impact on
> response times will increase. So instead of regular blips we have one
> large blip, less often. I think the latter will be worse, but welcome
> measurements that show that performance is smooth and regular with
> large files sizes.

Yeah. I don't think there's any way to get around the fact that there
will be bigger latency spikes in some cases with larger WAL files. I
think the question is whether they'll be common enough or serious
enough to worry about. For example, in a quick test on my laptop,
zero-filling a 16 megabyte file using "dd if=/dev/zero of=x bs=8k
count=2048" takes about 11 milliseconds, and zero-filling a 64
megabyte file with a count of 8192 increases the time to almost 50
milliseconds. That's something, but I wouldn't rate it as concerning.
There are a lot of things that can cause latency changes multiple
orders of magnitude larger than that, so worrying about that one in
particular would seem to me to be fairly pointless. However, that's
also a measurement on an unloaded system with an SSD, and the impact
may be a lot more on a big system where with lots of concurrent
activity, and if the process that does the write also has to do an
fsync, that will increase the cost considerably, too.

But the flip side is that it's wrong to imagine that there's no harm
in leaving the situation as it is. Even my MacBook Pro can crank out
about 2.7 WAL segments/second on "pgbench -c 16 -j 16 -T 60". I think
a decent server with a few more CPU cores than my laptop has could do
4-5 times that. So we shouldn't imagine that the costs of spewing out
a bajillion segment files are being paid only at the very high end.
Even somebody running PostgreSQL on a low-end virtual machine might
find it difficult to write an archive_command that can keep up if the
system is under continuous load. Of course, as Stephen pointed out,
there are toolkits that can do it and you should probably be using one
of those anyway for other reasons, but nevertheless spitting out
almost 3 WAL segments per second even on a laptop gives a whole new
meaning to the term "continuous archiving".

Another point to consider is that a bigger WAL segment size can
actually *improve* latency because every segment switch triggers an
immediate fsync, and every backend in the system ends up waiting for
it to finish. We should probably eventually try to push those flushes
into the background, and the zeroing work as well. My impression
(possibly incorrect?) is that we expect to settle into a routine where
zeroing new segments is relatively uncommon because we reuse old
segment files, but the forced end-of-segment flushes never go away.
So it's possible we might actually come out ahead on latency with this
change, at least sometimes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3 January 2017 at 15:44, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> Yeah. I don't think there's any way to get around the fact that there
> will be bigger latency spikes in some cases with larger WAL files.

One way would be for the WALwriter to zerofill new files ahead of
time, thus avoiding the latency spike.

> For example, in a quick test on my laptop,
> zero-filling a 16 megabyte file using "dd if=/dev/zero of=x bs=8k
> count=2048" takes about 11 milliseconds, and zero-filling a 64
> megabyte file with a count of 8192 increases the time to almost 50
> milliseconds. That's something, but I wouldn't rate it as concerning.

I would rate that as concerning, especially if we allow much larger sizes.

> But the flip side is that it's wrong to imagine that there's no harm
> in leaving the situation as it is.

The case for change has been made; the only discussion is what's in
the new patch.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jan 3, 2017 at 11:16 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 3 January 2017 at 15:44, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Yeah. I don't think there's any way to get around the fact that there
>> will be bigger latency spikes in some cases with larger WAL files.
>
> One way would be for the WALwriter to zerofill new files ahead of
> time, thus avoiding the latency spike.

Sure, we could do that. I think it's an independent improvement,
though: it is beneficial with or without this patch.

>> For example, in a quick test on my laptop,
>> zero-filling a 16 megabyte file using "dd if=/dev/zero of=x bs=8k
>> count=2048" takes about 11 milliseconds, and zero-filling a 64
>> megabyte file with a count of 8192 increases the time to almost 50
>> milliseconds. That's something, but I wouldn't rate it as concerning.
>
> I would rate that as concerning, especially if we allow much larger sizes.

I don't really understand the concern. If we allow large sizes but
they are not the default, people can make a throughput-vs-latency
trade-off when chosing a value for their installation. Those kind of
trade-offs are common and unavoidable. If we raise the default, then
it's more of a concern, but I'm not sure those numbers are big enough
to worry about. I'm not sure how to decide which numbers are big
enough to worry about, either.

I guess we need some test results showing what happens with this patch
in the real world before we go further. I agree that there's a
possible downside to raising the segment size, but my suspicion is
that the results are going to be better, not worse, because of
reducing the number of end-of-segment fsyncs. There's no point
worrying too much about how we're going to mitigate the negative
impact until we know for sure that there is one.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3 January 2017 at 16:24, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Jan 3, 2017 at 11:16 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On 3 January 2017 at 15:44, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> Yeah. I don't think there's any way to get around the fact that there
>>> will be bigger latency spikes in some cases with larger WAL files.
>>
>> One way would be for the WALwriter to zerofill new files ahead of
>> time, thus avoiding the latency spike.
>
> Sure, we could do that. I think it's an independent improvement,
> though: it is beneficial with or without this patch.

The latency spike problem is exacerbated by increasing file size, so I
think if we are allowing people to increase file size in this release
then we should fix the knock-on problem it causes in this release
also. If we don't fix it as part of this patch I would consider it an
open item.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jan 3, 2017 at 3:38 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 3 January 2017 at 16:24, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Jan 3, 2017 at 11:16 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> On 3 January 2017 at 15:44, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> Yeah. I don't think there's any way to get around the fact that there
>>>> will be bigger latency spikes in some cases with larger WAL files.
>>>
>>> One way would be for the WALwriter to zerofill new files ahead of
>>> time, thus avoiding the latency spike.
>>
>> Sure, we could do that. I think it's an independent improvement,
>> though: it is beneficial with or without this patch.
>
> The latency spike problem is exacerbated by increasing file size, so I
> think if we are allowing people to increase file size in this release
> then we should fix the knock-on problem it causes in this release
> also. If we don't fix it as part of this patch I would consider it an
> open item.

I think I'd like to see some benchmark results before forming an
opinion on whether that's a must-fix issue. I'm not sure I believe
that allowing a larger WAL segment size is going to make things worse
more than it makes them better. I think that should be tested, not
assumed true.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3 January 2017 at 21:33, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Jan 3, 2017 at 3:38 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On 3 January 2017 at 16:24, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Jan 3, 2017 at 11:16 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>>> On 3 January 2017 at 15:44, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>>> Yeah. I don't think there's any way to get around the fact that there
>>>>> will be bigger latency spikes in some cases with larger WAL files.
>>>>
>>>> One way would be for the WALwriter to zerofill new files ahead of
>>>> time, thus avoiding the latency spike.
>>>
>>> Sure, we could do that. I think it's an independent improvement,
>>> though: it is beneficial with or without this patch.
>>
>> The latency spike problem is exacerbated by increasing file size, so I
>> think if we are allowing people to increase file size in this release
>> then we should fix the knock-on problem it causes in this release
>> also. If we don't fix it as part of this patch I would consider it an
>> open item.
>
> I think I'd like to see some benchmark results before forming an
> opinion on whether that's a must-fix issue. I'm not sure I believe
> that allowing a larger WAL segment size is going to make things worse
> more than it makes them better. I think that should be tested, not
> assumed true.

Strange response. Nothing has been assumed. I asked for tests and you
provided measurements.

I suggest we fix just the problem as the fastest way forwards.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jan 4, 2017 at 3:05 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Strange response. Nothing has been assumed. I asked for tests and you
> provided measurements.

Sure, of zero-filling a file with dd. But I also pointed out that in
a real PostgreSQL cluster, the change could actually *reduce* latency.

> I suggest we fix just the problem as the fastest way forwards.

If you want to do the work, sure.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 4 January 2017 at 13:57, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jan 4, 2017 at 3:05 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Strange response. Nothing has been assumed. I asked for tests and you
>> provided measurements.
>
> Sure, of zero-filling a file with dd. But I also pointed out that in
> a real PostgreSQL cluster, the change could actually *reduce* latency.

I think we are talking at cross purposes. We agree that the main
change is useful, but it causes another problem which I can't see how
you can characterize as reduced latency, based upon your own
measurements.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jan 4, 2017 at 9:47 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 4 January 2017 at 13:57, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Wed, Jan 4, 2017 at 3:05 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> Strange response. Nothing has been assumed. I asked for tests and you
>>> provided measurements.
>>
>> Sure, of zero-filling a file with dd. But I also pointed out that in
>> a real PostgreSQL cluster, the change could actually *reduce* latency.
>
> I think we are talking at cross purposes. We agree that the main
> change is useful, but it causes another problem which I can't see how
> you can characterize as reduced latency, based upon your own
> measurements.

Zero-filling files will take longer if the files are bigger. That
will increase latency. But we will also have fewer forced
end-of-segment syncs. That will reduce latency. Which effect is
bigger?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jan 5, 2017 at 12:33 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jan 4, 2017 at 9:47 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On 4 January 2017 at 13:57, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Wed, Jan 4, 2017 at 3:05 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>>> Strange response. Nothing has been assumed. I asked for tests and you
>>>> provided measurements.
>>>
>>> Sure, of zero-filling a file with dd. But I also pointed out that in
>>> a real PostgreSQL cluster, the change could actually *reduce* latency.
>>
>> I think we are talking at cross purposes. We agree that the main
>> change is useful, but it causes another problem which I can't see how
>> you can characterize as reduced latency, based upon your own
>> measurements.
>
> Zero-filling files will take longer if the files are bigger. That
> will increase latency. But we will also have fewer forced
> end-of-segment syncs. That will reduce latency. Which effect is
> bigger?

It depends on if the environment is CPU-bounded or I/O bounded. If CPU
is at its limit, zero-filling takes time. If that's the I/O, fsync()
would take longer to complete.
--
Michael

On 4 January 2017 at 01:16, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Jan 3, 2017 at 6:23 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>> + /* Check if wal_segment_size is in the power of 2 */
>> + for (i = 0;; i++, pow2 = pow(2, i))
>> + if (pow2 >= wal_segment_size)
>> + break;
>> +
>> + if (wal_segment_size != 1 && pow2 > wal_segment_size)
>> + {
>> + fprintf(stderr, _("%s: WAL segment size must be in the power of 2\n"), progname);
>> + exit(1);
>> + }
>
> I recall taht pow(x, 2) and x * x result usually in the same assembly
> code, but pow() can never be more optimal than a simple
> multiplication. So I'd think that it is wiser to avoid it in this code
> path. Documentation is missing for the new replication command
> SHOW_WAL_SEG. Actually, why not just having an equivalent of the SQL
> command and be able to query parameter values?

This would probably be nicer written using a bitwise trick to ensure
that no lesser significant bits are set. If it's a power of 2, then
subtracting 1 should have all the lesser significant bits as 1, so
binary ANDing to that should be 0. i.e no common bits.

Something like:

/* ensure segment size is a power of 2 */
if ((wal_segment_size & (wal_segment_size - 1)) != 0)
{
fprintf(stderr, _("%s: WAL segment size must be in the power of
2\n"), progname);
exit(1);
}

There's a similar trick in bitmapset.c for RIGHTMOST_ONE, so looks
like we already have assumptions about two's complement arithmetic

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hello,

Thank you for your review.

On Tue, Jan 3, 2017 at 2:53 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:

> The following review has been posted through the commitfest application:
> make installcheck-world: not tested
> Implements feature: not tested
> Spec compliant: not tested
> Documentation: not tested
>
> General comments:
> There was some discussion about the impact of this on small installs,
> particularly around min_wal_size. The concern was that changing the default
> segment size to 64MB would significantly increase min_wal_size in terms of
> bytes. The default value for min_wal_size is 5 segments, so 16MB->64MB
> would mean going from 80MB to 320MB. IMHO if you're worried about that then
> just initdb with a smaller segment size. There's probably a number of other
> changes a small environment wants to make besides that. Perhaps it'd be
> worth making DEFAULT_XLOG_SEG_SIZE a configure option to better support
> that.
>

The patch maintains the current XLOG_SEG_SIZE of 16MB as the default. Only
the capability to change its value has been moved for configure to initdb.

>
> It's not clear from the thread that there is consensus that this feature
> is desired. In particular, the performance aspects of changing segment size
> from a C constant to a variable are in question. Someone with access to
> large hardware should test that. Andres[1] and Robert[2] did suggest that
> the option could be changed to a bitshift, which IMHO would also solve some
> sanity-checking issues.
>
> + * initdb passes the WAL segment size in an environment variable.
> We don't
> + * bother doing any sanity checking, we already check in initdb
> that the
> + * user gives a sane value.
>
> That doesn't seem like a good idea to me. If anything, the backend should
> sanity-check and initdb just rely on that. Perhaps this is how other initdb
> options work, but it still seems bogus. In particular, verifying the size
> is a power of 2 seems important, as failing that would probably be
> ReallyBad(tm).
>
> The patch also blindly trusts the value read from the control file; I'm
> not sure if that's standard procedure or not, but ISTM it'd be worth
> sanity-checking that as well.
>

There is a CRC check to detect error in the file. I think all the
ControlFile values are used directly and not re-verified.

>
> The patch leaves the base GUC units for min_wal_size and max_wal_size as
> the # of segments. I'm not sure if that's a great idea.
>

I think we can leave it as is. This is used in CalculateCheckpontSegments
and in XLOGfileslop to calculate the segment numbers.

> + * convert_unit
> + *
> + * This takes the value in kbytes and then returns value in user-readable
> format
>
> This function needs a more specific name, such as pretty_print_kb().
>

I agree pretty_print_kb would have been a better for this function.
However, I have realised that using the show hook and this function is not
suitable and have found a better way of handling the removal of
GUC_UNIT_XSEGS which no longer needs this function : using the GUC_UNIT_KB,
convert the value in bytes to wal segment count instead in the assign hook.
The next version of patch will use this.

>
> + /* Check if wal_segment_size is in the power of 2 */
> + for (i = 0;; i++, pow2 = pow(2, i))
> + if (pow2 >= wal_segment_size)
> + break;
> +
> + if (wal_segment_size != 1 && pow2 > wal_segment_size)
> + {
> + fprintf(stderr, _("%s: WAL segment size must be in
> the power of 2\n"), progname);
> + exit(1);
> + }
>
> IMHO it'd be better to use the n & (n-1) check detailed at [3].
>

Yes, even I had come across it. I will incorporate this in the next version
of the patch.

>
> Actually, there's got to be other places that need to check this, so it'd
> be nice to just create a function that verifies a number is a power of 2.
>
> + if (log_fname != NULL)
> + XLogFromFileName(log_fname, &minXlogTli, &minXlogSegNo);
> +
>
> Please add a comment about why XLogFromFileName has to be delayed.
>

Oh yes!.

>
> /*
> + * DEFAULT_XLOG_SEG_SIZE is the size of a single WAL file. This must be
> a power
> + * of 2 and larger than XLOG_BLCKSZ (preferably, a great deal larger than
> + * XLOG_BLCKSZ).
> + *
> + * Changing DEFAULT_XLOG_SEG_SIZE requires an initdb.
> + */
> +#define DEFAULT_XLOG_SEG_SIZE (16*1024*1024)
>
> That comment isn't really accurate. It would be more useful to explain
> that DEFAULT_XLOG_SEG_SIZE is the default size of a WAL segment used by
> initdb if a different value isn't specified.
>

I will correct this comment

The new version of the patch will be posted soon.

Thank you,

Beena Emerson

Have a Great Day!

Hello,

On Tue, Jan 3, 2017 at 5:46 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Tue, Jan 3, 2017 at 6:23 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>
> wrote:
> > + /* Check if wal_segment_size is in the power of 2 */
> > + for (i = 0;; i++, pow2 = pow(2, i))
> > + if (pow2 >= wal_segment_size)
> > + break;
> > +
> > + if (wal_segment_size != 1 && pow2 > wal_segment_size)
> > + {
> > + fprintf(stderr, _("%s: WAL segment size must be
> in the power of 2\n"), progname);
> > + exit(1);
> > + }
>
> I recall taht pow(x, 2) and x * x result usually in the same assembly
> code, but pow() can never be more optimal than a simple
> multiplication. So I'd think that it is wiser to avoid it in this code
> path. Documentation is missing for the new replication command
> SHOW_WAL_SEG.

As mentioned earlier, documents are not fully updated.

> Actually, why not just having an equivalent of the SQL
> command and be able to query parameter values?
>

This patch only needed the wal_segment_size and hence I made this specific
command.
How often and why would we need other parameter values in the replication
connection?
Making it a more general command to fetch any parameter can be a separate
topic. If it gets consensus, maybe it could be done and used here.

Thank you,

Beena Emerson

Have a Great Day!

On Thu, Jan 5, 2017 at 6:39 AM, Beena Emerson <memissemerson(at)gmail(dot)com> wrote:
> This patch only needed the wal_segment_size and hence I made this specific
> command.
> How often and why would we need other parameter values in the replication
> connection?
> Making it a more general command to fetch any parameter can be a separate
> topic. If it gets consensus, maybe it could be done and used here.

I think the idea of supporting SHOW here is better than adding a
special-purpose command just for the WAL size.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jan 5, 2017 at 8:39 PM, Beena Emerson <memissemerson(at)gmail(dot)com> wrote:
> On Tue, Jan 3, 2017 at 5:46 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
>> Actually, why not just having an equivalent of the SQL
>> command and be able to query parameter values?
>
> This patch only needed the wal_segment_size and hence I made this specific
> command.
> How often and why would we need other parameter values in the replication
> connection?
> Making it a more general command to fetch any parameter can be a separate
> topic. If it gets consensus, maybe it could be done and used here.

I concur that for this patch it may not be necessary. But let's not
narrow us in a corner when designing things. Being able to query the
value of parameters is something that I think is actually useful for
cases where custom GUCs are loaded on the server's
shared_preload_libraries to do validation checks (one case is a
logical decoder on backend, with streaming receiver as client
expecting the logical decoder to do a minimum). This can allow a
client to do checks only using a replication stream. Another case that
I have in mind is for utilities like pg_rewind, we have been
discussing about being able to not need a superuser when querying the
target server. Having such a command would allow for example pg_rewind
to do a 'SHOW full_page_writes' without the need of an extra
connection.
--
Michael

On Fri, Jan 6, 2017 at 11:36 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Thu, Jan 5, 2017 at 8:39 PM, Beena Emerson <memissemerson(at)gmail(dot)com>
> wrote:
> > On Tue, Jan 3, 2017 at 5:46 PM, Michael Paquier <
> michael(dot)paquier(at)gmail(dot)com>
> > wrote:
> >> Actually, why not just having an equivalent of the SQL
> >> command and be able to query parameter values?
> >
> > This patch only needed the wal_segment_size and hence I made this
> specific
> > command.
> > How often and why would we need other parameter values in the replication
> > connection?
> > Making it a more general command to fetch any parameter can be a separate
> > topic. If it gets consensus, maybe it could be done and used here.
>
> I concur that for this patch it may not be necessary. But let's not
> narrow us in a corner when designing things. Being able to query the
> value of parameters is something that I think is actually useful for
> cases where custom GUCs are loaded on the server's
> shared_preload_libraries to do validation checks (one case is a
> logical decoder on backend, with streaming receiver as client
> expecting the logical decoder to do a minimum). This can allow a
> client to do checks only using a replication stream. Another case that
> I have in mind is for utilities like pg_rewind, we have been
> discussing about being able to not need a superuser when querying the
> target server. Having such a command would allow for example pg_rewind
> to do a 'SHOW full_page_writes' without the need of an extra
> connection.
>
>
I see the point. I will change the SHOW_WAL_SEGSZ to a general SHOW command
in the next version of the patch.

Thank you,

Beena Emerson

Have a Great Day!

On Fri, Jan 6, 2017 at 6:32 PM, Beena Emerson <memissemerson(at)gmail(dot)com> wrote:
> I see the point. I will change the SHOW_WAL_SEGSZ to a general SHOW command
> in the next version of the patch.

Could you split things? There could be one patch to introduce the SHOW
command, and one on top of it for your patch to be able to change the
WAL segment size wiht initdb.
--
Michael

On 1/4/17 10:03 PM, David Rowley wrote:
>> I recall taht pow(x, 2) and x * x result usually in the same assembly
>> code, but pow() can never be more optimal than a simple
>> multiplication. So I'd think that it is wiser to avoid it in this code
>> path. Documentation is missing for the new replication command
>> SHOW_WAL_SEG. Actually, why not just having an equivalent of the SQL
>> command and be able to query parameter values?
> This would probably be nicer written using a bitwise trick to ensure
> that no lesser significant bits are set. If it's a power of 2, then
> subtracting 1 should have all the lesser significant bits as 1, so
> binary ANDing to that should be 0. i.e no common bits.
>
> Something like:
>
> /* ensure segment size is a power of 2 */
> if ((wal_segment_size & (wal_segment_size - 1)) != 0)
> {
> fprintf(stderr, _("%s: WAL segment size must be in the power of
> 2\n"), progname);
> exit(1);
> }
>
> There's a similar trick in bitmapset.c for RIGHTMOST_ONE, so looks
> like we already have assumptions about two's complement arithmetic

Well, now that there's 3 places that need to do almost the same thing, I
think it'd be best to just centralize this somewhere. I realize that's
not going to save any significant amount of code, but it would make it
crystal clear what's going on (assuming the excellent comment above
RIGHTMOST_ONE was kept).
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)

On 1/5/17 5:38 AM, Beena Emerson wrote:
> On Tue, Jan 3, 2017 at 2:53 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com
> <mailto:Jim(dot)Nasby(at)bluetreble(dot)com>> wrote:
> General comments:
> There was some discussion about the impact of this on small
> installs, particularly around min_wal_size. The concern was that
...
> The patch maintains the current XLOG_SEG_SIZE of 16MB as the default.
> Only the capability to change its value has been moved for configure to
> initdb.

Ah, I missed that. Thanks for clarifying.

> It's not clear from the thread that there is consensus that this
> feature is desired. In particular, the performance aspects of
> changing segment size from a C constant to a variable are in
> question. Someone with access to large hardware should test that.
> Andres[1] and Robert[2] did suggest that the option could be changed
> to a bitshift, which IMHO would also solve some sanity-checking issues.

Are you going to change to a bitshift in the next patch?

> + * initdb passes the WAL segment size in an environment
> variable. We don't
> + * bother doing any sanity checking, we already check in
> initdb that the
> + * user gives a sane value.
>
> That doesn't seem like a good idea to me. If anything, the backend
> should sanity-check and initdb just rely on that. Perhaps this is
> how other initdb options work, but it still seems bogus. In
> particular, verifying the size is a power of 2 seems important, as
> failing that would probably be ReallyBad(tm).
>
> The patch also blindly trusts the value read from the control file;
> I'm not sure if that's standard procedure or not, but ISTM it'd be
> worth sanity-checking that as well.
>
>
> There is a CRC check to detect error in the file. I think all the
> ControlFile values are used directly and not re-verified.

Sounds good. I do still think the variable from initdb should be
sanity-checked.

> The patch leaves the base GUC units for min_wal_size and
> max_wal_size as the # of segments. I'm not sure if that's a great idea.
>
>
> I think we can leave it as is. This is used in
> CalculateCheckpontSegments and in XLOGfileslop to calculate the segment
> numbers.

My concern here is that we just changed from segments to KB for all the
checkpoint settings, and this is introducing segments back in, but ...

> I agree pretty_print_kb would have been a better for this function.
> However, I have realised that using the show hook and this function is
> not suitable and have found a better way of handling the removal of
> GUC_UNIT_XSEGS which no longer needs this function : using the
> GUC_UNIT_KB, convert the value in bytes to wal segment count instead in
> the assign hook. The next version of patch will use this.

... it sounds like you're going back to exposing KB to users, and that's
all that really matters.

> IMHO it'd be better to use the n & (n-1) check detailed at [3].

See my other email about that.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)

On Sat, Jan 7, 2017 at 7:45 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> Well, now that there's 3 places that need to do almost the same thing, I
> think it'd be best to just centralize this somewhere. I realize that's not
> going to save any significant amount of code, but it would make it crystal
> clear what's going on (assuming the excellent comment above RIGHTMOST_ONE
> was kept).

Hmm. This sounds a lot like what fls() and my_log2() also do. I've
been quietly advocating for fls() because we only provide an
implementation in src/port if the operating system doesn't have it,
and the operating system may have an implementation that optimizes to
a single machine-language instruction (bsrl on x86, I think, see
4f658dc851a73fc309a61be2503c29ed78a1592e). But the fact that our
src/port implementation uses a loop instead of the RIGHTMOST_ONE()
trick seems non-optimal.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 10 January 2017 at 07:40, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Jan 7, 2017 at 7:45 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>> Well, now that there's 3 places that need to do almost the same thing, I
>> think it'd be best to just centralize this somewhere. I realize that's not
>> going to save any significant amount of code, but it would make it crystal
>> clear what's going on (assuming the excellent comment above RIGHTMOST_ONE
>> was kept).
>
> Hmm. This sounds a lot like what fls() and my_log2() also do. I've
> been quietly advocating for fls() because we only provide an
> implementation in src/port if the operating system doesn't have it,
> and the operating system may have an implementation that optimizes to
> a single machine-language instruction (bsrl on x86, I think, see
> 4f658dc851a73fc309a61be2503c29ed78a1592e). But the fact that our
> src/port implementation uses a loop instead of the RIGHTMOST_ONE()
> trick seems non-optimal.

It does really sound like we need a bitutils.c as mentioned in [1].
It would be good to make use of GCC's __builtin_popcount [2] instead
of the number_of_ones[] array in bitmapset.c. It should be a bit
faster and less cache polluting.

[1] https://www.postgresql.org/message-id/14578.1462595165@sss.pgh.pa.us
[2] https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Jan 8, 2017 at 9:52 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>> I agree pretty_print_kb would have been a better for this function.
>> However, I have realised that using the show hook and this function is
>> not suitable and have found a better way of handling the removal of
>> GUC_UNIT_XSEGS which no longer needs this function : using the
>> GUC_UNIT_KB, convert the value in bytes to wal segment count instead in
>> the assign hook. The next version of patch will use this.
>
>
> ... it sounds like you're going back to exposing KB to users, and that's all
> that really matters.
>
>> IMHO it'd be better to use the n & (n-1) check detailed at [3].

That would be better.

So I am looking at the proposed patch, though there have been reviews
the patch was in "Needs Review" state, and as far as I can see it is a
couple of things for frontends. Just by grepping for XLOG_SEG_SIZE I
have spotted the following problems:
- pg_standby uses it to know about the next segment available.
- pg_receivexlog still uses it in segment handling.
It may be a good idea to just remove XLOG_SEG_SIZE and fix the code
paths that fail to compile without it, frontend utilities included
because a lot of them now rely on the value coded in xlog_internal.h,
but with this patch the value is set up in the context of initdb. And
this would induce major breakages in many backup tools, pg_rman coming
first in mind... We could replace it with for example a macro that
frontends could use to check if the size of the WAL segment is in a
valid range if the tool does not have direct access to the Postgres
instance (aka the size of the WAL segment used there) as there are as
well offline tools.

-#define XLogSegSize ((uint32) XLOG_SEG_SIZE)
+
+extern uint32 XLogSegSize;
+#define XLOG_SEG_SIZE XLogSegSize
This bit is really bad for frontend declaring xlog_internal.h...

--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -62,7 +62,7 @@ static const char *progname;

static int secs_per_test = 5;
static int needs_unlink = 0;
-static char full_buf[XLOG_SEG_SIZE],
+static char full_buf[DEFAULT_XLOG_SEG_SIZE],
This would make sense as a new option of pg_test_fsync.

A performance study would be a good idea as well. Regarding the
generic SHOW command in the replication protocol, I may do it for next
CF, I have use cases for it in my pocket.
--
Michael

On Tue, Jan 17, 2017 at 12:18 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com
> wrote:

> On Sun, Jan 8, 2017 at 9:52 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>
> wrote:
> >> I agree pretty_print_kb would have been a better for this function.
> >> However, I have realised that using the show hook and this function is
> >> not suitable and have found a better way of handling the removal of
> >> GUC_UNIT_XSEGS which no longer needs this function : using the
> >> GUC_UNIT_KB, convert the value in bytes to wal segment count instead in
> >> the assign hook. The next version of patch will use this.
> >
> >
> > ... it sounds like you're going back to exposing KB to users, and that's
> all
> > that really matters.
> >
> >> IMHO it'd be better to use the n & (n-1) check detailed at [3].
>
> That would be better.
>
> So I am looking at the proposed patch, though there have been reviews
> the patch was in "Needs Review" state, and as far as I can see it is a
> couple of things for frontends. Just by grepping for XLOG_SEG_SIZE I
> have spotted the following problems:
> - pg_standby uses it to know about the next segment available.
>

Yes. I am aware of this and had mentioned it in my post.

> - pg_receivexlog still uses it in segment handling.
> It may be a good idea to just remove XLOG_SEG_SIZE and fix the code
> paths that fail to compile without it, frontend utilities included
> because a lot of them now rely on the value coded in xlog_internal.h,
> but with this patch the value is set up in the context of initdb. And
> this would induce major breakages in many backup tools, pg_rman coming
> first in mind... We could replace it with for example a macro that
> frontends could use to check if the size of the WAL segment is in a
> valid range if the tool does not have direct access to the Postgres
> instance (aka the size of the WAL segment used there) as there are as
> well offline tools.
>

I will see whats the best way to do this.

>
> -#define XLogSegSize ((uint32) XLOG_SEG_SIZE)
> +
> +extern uint32 XLogSegSize;
> +#define XLOG_SEG_SIZE XLogSegSize
> This bit is really bad for frontend declaring xlog_internal.h...
>
> --- a/src/bin/pg_test_fsync/pg_test_fsync.c
> +++ b/src/bin/pg_test_fsync/pg_test_fsync.c
> @@ -62,7 +62,7 @@ static const char *progname;
>
> static int secs_per_test = 5;
> static int needs_unlink = 0;
> -static char full_buf[XLOG_SEG_SIZE],
> +static char full_buf[DEFAULT_XLOG_SEG_SIZE],
> This would make sense as a new option of pg_test_fsync.
>
> A performance study would be a good idea as well. Regarding the
> generic SHOW command in the replication protocol, I may do it for next
> CF, I have use cases for it in my pocket.
>
>
Thank you for your review.

I have already made patch for the generic SHOW replication command
(attached) and am working on the new initdb patch based on that.
I have not yet fixed the pg_standby issue. I am trying to address all the
comments and bugs still.

--

Beena Emerson

Have a Great Day!

On Tue, Jan 17, 2017 at 4:06 PM, Beena Emerson <memissemerson(at)gmail(dot)com> wrote:
> I have already made patch for the generic SHOW replication command
> (attached) and am working on the new initdb patch based on that.
> I have not yet fixed the pg_standby issue. I am trying to address all the
> comments and bugs still.

Having documentation for this patch in protocol.sgml would be nice.
--
Michael

On Tue, Jan 17, 2017 at 12:38 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com
> wrote:

> On Tue, Jan 17, 2017 at 4:06 PM, Beena Emerson <memissemerson(at)gmail(dot)com>
> wrote:
> > I have already made patch for the generic SHOW replication command
> > (attached) and am working on the new initdb patch based on that.
> > I have not yet fixed the pg_standby issue. I am trying to address all the
> > comments and bugs still.
>
> Having documentation for this patch in protocol.sgml would be nice.
>

Yes. I will add that.

--
Thank you,

Beena Emerson

Have a Great Day!

On Tue, Jan 17, 2017 at 12:50 PM, Beena Emerson <memissemerson(at)gmail(dot)com>
wrote:

>
>
> On Tue, Jan 17, 2017 at 12:38 PM, Michael Paquier <
> michael(dot)paquier(at)gmail(dot)com> wrote:
>
>> On Tue, Jan 17, 2017 at 4:06 PM, Beena Emerson <memissemerson(at)gmail(dot)com>
>> wrote:
>> > I have already made patch for the generic SHOW replication command
>> > (attached) and am working on the new initdb patch based on that.
>> > I have not yet fixed the pg_standby issue. I am trying to address all
>> the
>> > comments and bugs still.
>>
>> Having documentation for this patch in protocol.sgml would be nice.
>>
>
> Yes. I will add that.
>
>
>
PFA the patch with the documentation included.

--
Thank you,

Beena Emerson

Have a Great Day!

On Tue, Jan 17, 2017 at 7:19 PM, Beena Emerson <memissemerson(at)gmail(dot)com> wrote:
> PFA the patch with the documentation included.

It is usually better to keep doc lines under control of 72-80
characters if possible.

+ /* column 1: Wal segment size */
+ len = strlen(value);
+ pq_sendint(&buf, len, 4);
+ pq_sendbytes(&buf, value, len);
Bip. Error. This is a parameter value, not the WAL segment size.

Except those minor points this works as expected, and is consistent
with non-replication sessions, so that's nice by itself:
=# create user toto replication login;
CREATE ROLE

$ psql "replication=1" -U toto
=> SHOW foo;
ERROR: 42704: unrecognized configuration parameter "foo"
LOCATION: GetConfigOptionByName, guc.c:7968
Time: 0.245 ms
=> SHOW log_directory;
ERROR: 42501: must be superuser to examine "log_directory"
LOCATION: GetConfigOptionByName, guc.c:7974

I think that we could get a committer look at that at the least.
--
Michael

On Tue, Jan 17, 2017 at 8:54 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> I think that we could get a committer look at that at the least.

This is sort of awkward, because it would be nice to reuse the code
for the existing SHOW command rather than reinventing the wheel, but
it's not very easy to do that, primarily because there are a number of
places which rely on being able to do catalog access, which is not
possible with a replication connection in hand. I got it working
after hacking various things, so I have a complete list of the
problems involved:

1. ShowGUCConfigOption() calls TupleDescInitEntry(), which does a
catcache lookup to get the types pg_type entry. This isn't any big
problem; I hacked around it by adding a TupleDescInitBuiltinEntry()
which knows about the types that guc.c (and likely other builtins)
care about.

2. SendRowDescriptionMessage calls getBaseTypeAndTypmod(), which does
a catcache lookup to figure out whether the type is a domain. I
short-circuited it by having it assume anything with an OID less than
10000 is not a domain.

3. printtup_prepare_info calls getTypeOutputInfo(), which does a
catcache lookup to figure out the type output function's OID and
whether it's a varlena. I bypassed that with an unspeakable hack.

4. printtup.c's code in general assumes that a DR_printtup always has
a portal. It doesn't seem to mind if the portal doesn't contain
anything very meaningful, but it has to have one. This problem has
nothing to do with catalog access, but it's a problem. I solved it by
(surprise) creating a portal, but I am not sure that's a very good
idea.

Problems 2-4 actually have to do with a DestReceiver of type
DestRemote really, really wanting to have an associated Portal and
database connection, so one approach is to create a stripped-down
DestReceiver that doesn't care about those things and then passing
that to GetPGVariable. That's not any less code than the way Beena
coded it, of course; it's probably more. On the other hand, the
stripped-down DestReciever implementation is more likely to be usable
the next time somebody wants to add a new replication command, whereas
this ad-hoc code to directly construct protocol messages will not be
reusable.

Opinions? (Hacked-up patch attached for educational purposes.)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jan 18, 2017 at 12:42 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Problems 2-4 actually have to do with a DestReceiver of type
> DestRemote really, really wanting to have an associated Portal and
> database connection, so one approach is to create a stripped-down
> DestReceiver that doesn't care about those things and then passing
> that to GetPGVariable.

I tried that and it worked out pretty well, so I'm inclined to go with
this approach. Proposed patches attached. 0001 adds the new
DestReceiver type, and 0002 is a revised patch to implement the SHOW
command itself.

Thoughts, comments?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jan 20, 2017 at 12:49 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jan 18, 2017 at 12:42 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Problems 2-4 actually have to do with a DestReceiver of type
>> DestRemote really, really wanting to have an associated Portal and
>> database connection, so one approach is to create a stripped-down
>> DestReceiver that doesn't care about those things and then passing
>> that to GetPGVariable.
>
> I tried that and it worked out pretty well, so I'm inclined to go with
> this approach. Proposed patches attached. 0001 adds the new
> DestReceiver type, and 0002 is a revised patch to implement the SHOW
> command itself.
>
> Thoughts, comments?

This looks like a sensible approach to me. DestRemoteSimple could be
useful for background workers that are not connected to a database as
well. Isn't there a problem with PGC_REAL parameters?
--
Michael

On Fri, Jan 20, 2017 at 2:34 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Jan 20, 2017 at 12:49 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Wed, Jan 18, 2017 at 12:42 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> Problems 2-4 actually have to do with a DestReceiver of type
>>> DestRemote really, really wanting to have an associated Portal and
>>> database connection, so one approach is to create a stripped-down
>>> DestReceiver that doesn't care about those things and then passing
>>> that to GetPGVariable.
>>
>> I tried that and it worked out pretty well, so I'm inclined to go with
>> this approach. Proposed patches attached. 0001 adds the new
>> DestReceiver type, and 0002 is a revised patch to implement the SHOW
>> command itself.
>>
>> Thoughts, comments?
>
> This looks like a sensible approach to me. DestRemoteSimple could be
> useful for background workers that are not connected to a database as
> well. Isn't there a problem with PGC_REAL parameters?

No, because the output of SHOW is always of type text, regardless of
the type of the GUC.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Jan 21, 2017 at 4:50 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jan 20, 2017 at 2:34 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Fri, Jan 20, 2017 at 12:49 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Wed, Jan 18, 2017 at 12:42 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> Problems 2-4 actually have to do with a DestReceiver of type
>>>> DestRemote really, really wanting to have an associated Portal and
>>>> database connection, so one approach is to create a stripped-down
>>>> DestReceiver that doesn't care about those things and then passing
>>>> that to GetPGVariable.
>>>
>>> I tried that and it worked out pretty well, so I'm inclined to go with
>>> this approach. Proposed patches attached. 0001 adds the new
>>> DestReceiver type, and 0002 is a revised patch to implement the SHOW
>>> command itself.
>>>
>>> Thoughts, comments?
>>
>> This looks like a sensible approach to me. DestRemoteSimple could be
>> useful for background workers that are not connected to a database as
>> well. Isn't there a problem with PGC_REAL parameters?
>
> No, because the output of SHOW is always of type text, regardless of
> the type of the GUC.

Thinking about that over night, that looks pretty nice. What would be
nicer though would be to add INT8OID and BYTEAOID in the list, and
convert as well the other replication commands. At the end, I think
that we should finish by being able to remove all pq_* routine
dependencies in walsender.c, saving quite a couple of lines.
--
Michael

Hello,

Please find attached an updated WIP patch. I have incorporated almost all
comments. This is to be applied over Robert's patches. I will post
performance results later on.

1. shift (>>) and AND (&) operations: The assign hook of wal_segment_size
sets the WalModMask and WalShiftBit. All the modulo and division operations
using XLogSegSize has been replaced with these. However, there are many
preprocessors which divide with XLogSegSize in xlog_internal.h. I have not
changed these because it would mean I will have to reassign the WalShiftBit
along with XLogSegSize in all the modules which use these macros. That does
not seem to be a good idea. Also, this means shift operator can be used
only in couple of places.

2. pg_standby: it deals with WAL files, so I have used the file size to set
the XLogSegSize (similar to pg_xlogdump). Also, macro
MaxSegmentsPerLogFile using XLOG_SEG_SIZE is now defined
in SetWALFileNameForCleanup where it is used. Since XLOG_SEG_SIZE is not
preset, the code which throws an message if the file size is greater than
XLOG_SEG_SIZE had to be removed.

3. XLOGChooseNumBuffers: This function, called during the creation of
Shared Memory Segment, requires XLogSegSize which is set from the
ControlFile. Hence temporarily read the ControlFile in XLOGShmemSize before
invoking XLOGChooseNumBuffer. The ControlFile is read again and stored on
the Shared Memory later on.

4. IsValidXLogSegSize: This is a macro to verify the XLogSegSize. This is
used in initdb, pg_xlogdump, pg_standby.

5. Macro for power2: There were couple of ideas to make it centralised.
For now, I have just defined it in xlog_internal.

6. Since CRC is used to verify the ControlFile before reading all the
contents from it as is and I do not see the need to re-verify the
xlog_seg_size.

7. min/max_wal_size still take values in KB unit and internally store it as
segment count. Though the calculation is now shifted to their respective
assign hook as the GUC_UNIT_XSEGS had to be removed.

8. Declaring XLogSegSize: There are 2 internal variables for the same
parameter. In original code XLOG_SEG_SIZE is defined in the auto-generated
file src/include/pg_config.h. And xlog_internal.h defines:

#define XLogSegSize ((uint32) XLOG_SEG_SIZE)

To avoid renaming all parts of code, I made the following change in
xlog_internal.h

+ extern uint32 XLogSegSize;

+#define XLOG_SEG_SIZE XLogSegSize

would it be better to just use one variable XLogSegSize everywhere. But
few external modules could be using XLOG_SEG_SIZE. Thoughts?

9. Documentation will be added in next version of patch.
--

Beena Emerson

On Sat, Jan 21, 2017 at 5:30 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Sat, Jan 21, 2017 at 4:50 AM, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
> > On Fri, Jan 20, 2017 at 2:34 AM, Michael Paquier
> > <michael(dot)paquier(at)gmail(dot)com> wrote:
> >> On Fri, Jan 20, 2017 at 12:49 AM, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
> >>> On Wed, Jan 18, 2017 at 12:42 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
> >>>> Problems 2-4 actually have to do with a DestReceiver of type
> >>>> DestRemote really, really wanting to have an associated Portal and
> >>>> database connection, so one approach is to create a stripped-down
> >>>> DestReceiver that doesn't care about those things and then passing
> >>>> that to GetPGVariable.
> >>>
> >>> I tried that and it worked out pretty well, so I'm inclined to go with
> >>> this approach. Proposed patches attached. 0001 adds the new
> >>> DestReceiver type, and 0002 is a revised patch to implement the SHOW
> >>> command itself.
> >>>
> >>> Thoughts, comments?
> >>
> >> This looks like a sensible approach to me. DestRemoteSimple could be
> >> useful for background workers that are not connected to a database as
> >> well. Isn't there a problem with PGC_REAL parameters?
> >
> > No, because the output of SHOW is always of type text, regardless of
> > the type of the GUC.
>
> Thinking about that over night, that looks pretty nice. What would be
> nicer though would be to add INT8OID and BYTEAOID in the list, and
> convert as well the other replication commands. At the end, I think
> that we should finish by being able to remove all pq_* routine
> dependencies in walsender.c, saving quite a couple of lines.
> --
> Michael
>

--
Thank you,

Beena Emerson

Have a Great Day!

On Fri, Jan 20, 2017 at 7:00 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
>> No, because the output of SHOW is always of type text, regardless of
>> the type of the GUC.
>
> Thinking about that over night, that looks pretty nice. What would be
> nicer though would be to add INT8OID and BYTEAOID in the list, and
> convert as well the other replication commands. At the end, I think
> that we should finish by being able to remove all pq_* routine
> dependencies in walsender.c, saving quite a couple of lines.

Might be worth investigating, but I don't feel any obligation to do
that right now. Thanks for the review; committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jan 25, 2017 at 6:58 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jan 20, 2017 at 7:00 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> No, because the output of SHOW is always of type text, regardless of
>>> the type of the GUC.
>>
>> Thinking about that over night, that looks pretty nice. What would be
>> nicer though would be to add INT8OID and BYTEAOID in the list, and
>> convert as well the other replication commands. At the end, I think
>> that we should finish by being able to remove all pq_* routine
>> dependencies in walsender.c, saving quite a couple of lines.
>
> Might be worth investigating, but I don't feel any obligation to do
> that right now. Thanks for the review; committed.

OK, I have done this refactoring effort as attached because I think
that's really worth it. And here are the diff numbers:
3 files changed, 113 insertions(+), 162 deletions(-)
That's a bit less than what I thought first because of all the
singularities of bytea in its output and the way TIMELINE_HISTORY
takes advantage of the message level routines. Still for
IDENTIFY_SYSTEM, START_REPLICATION and CREATE_REPLICATION_SLOT the
gains in readability are here.

What do you think?
--
Michael

On Tue, Jan 24, 2017 at 10:26 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Jan 25, 2017 at 6:58 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Jan 20, 2017 at 7:00 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>> No, because the output of SHOW is always of type text, regardless of
>>>> the type of the GUC.
>>>
>>> Thinking about that over night, that looks pretty nice. What would be
>>> nicer though would be to add INT8OID and BYTEAOID in the list, and
>>> convert as well the other replication commands. At the end, I think
>>> that we should finish by being able to remove all pq_* routine
>>> dependencies in walsender.c, saving quite a couple of lines.
>>
>> Might be worth investigating, but I don't feel any obligation to do
>> that right now. Thanks for the review; committed.
>
> OK, I have done this refactoring effort as attached because I think
> that's really worth it. And here are the diff numbers:
> 3 files changed, 113 insertions(+), 162 deletions(-)
> That's a bit less than what I thought first because of all the
> singularities of bytea in its output and the way TIMELINE_HISTORY
> takes advantage of the message level routines. Still for
> IDENTIFY_SYSTEM, START_REPLICATION and CREATE_REPLICATION_SLOT the
> gains in readability are here.

Seems OK to me, but I think I'd want to hear a few other opinions
before committing it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2017-01-26 13:16:13 -0500, Robert Haas wrote:
> > OK, I have done this refactoring effort as attached because I think
> > that's really worth it. And here are the diff numbers:
> > 3 files changed, 113 insertions(+), 162 deletions(-)
> > That's a bit less than what I thought first because of all the
> > singularities of bytea in its output and the way TIMELINE_HISTORY
> > takes advantage of the message level routines. Still for
> > IDENTIFY_SYSTEM, START_REPLICATION and CREATE_REPLICATION_SLOT the
> > gains in readability are here.
>
> Seems OK to me, but I think I'd want to hear a few other opinions
> before committing it.

Just to be absolutely sure: We're talking about Michael's cleanup patch,
not the thread's original topic?

Andres

On Thu, Jan 26, 2017 at 1:34 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2017-01-26 13:16:13 -0500, Robert Haas wrote:
>> > OK, I have done this refactoring effort as attached because I think
>> > that's really worth it. And here are the diff numbers:
>> > 3 files changed, 113 insertions(+), 162 deletions(-)
>> > That's a bit less than what I thought first because of all the
>> > singularities of bytea in its output and the way TIMELINE_HISTORY
>> > takes advantage of the message level routines. Still for
>> > IDENTIFY_SYSTEM, START_REPLICATION and CREATE_REPLICATION_SLOT the
>> > gains in readability are here.
>>
>> Seems OK to me, but I think I'd want to hear a few other opinions
>> before committing it.
>
> Just to be absolutely sure: We're talking about Michael's cleanup patch,
> not the thread's original topic?

Correct.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company