Re: [9.4] Make full_page_writes only settable on server start?

There is a significant amount of code supporting the changing of
full_page_writes while the server is running, including an
XLOG_FPW_CHANGE wal record. But I don't see the use-case; surely almost
nobody changes this on a running server, because either your filesystem
guarantees atomic page writes for you or not.

I'd like to submit a patch to just make it into a PGC_POSTMASTER and
remove the code to support changing it.

Then, I intend to write another patch to make the full-page writes for
checksums honor the full_page_writes setting. That will be easier to
write once it's a PGC_POSTMASTER.

Any reason to keep the setting as a PGC_SIGHUP?

Regards,
Jeff Davis

On Mon, Sep 2, 2013 at 7:10 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> I'd like to submit a patch to just make it into a PGC_POSTMASTER and
> remove the code to support changing it.

Makes sense to me.

I wonder, is anyone really running in production with full_page_writes
off? I talked to someone a while ago who used Postgres on ZFS with
data journaling, and was perfectly well aware of the fact that
theoretically he could safely turn the setting off, and yet chose not
to. Now, I know that Greg Smith's book describes the conditions in
which it's acceptable and the precautions that should be taken and so
on, but in my (admittedly relatively limited) experience, no one
actually does it in production. My sample size for people who at least
considered doing it (that both believed that Postgres could write
pages atomically on their hardware/FS, and also knew that
full_page_writes exists and what it means) is only 1.

At least in people's minds, it might be that the knowledge that no one
runs with full_page_writes off is enough to put them off. It's like
building Postgres with a non-standard BLCKSZ -- even if you had solid
evidence that it would help performance in your case, would you really
want to do it?

--
Peter Geoghegan

On 2013-09-02 19:10:57 -0700, Jeff Davis wrote:
> There is a significant amount of code supporting the changing of
> full_page_writes while the server is running, including an
> XLOG_FPW_CHANGE wal record. But I don't see the use-case; surely almost
> nobody changes this on a running server, because either your filesystem
> guarantees atomic page writes for you or not.
>
> I'd like to submit a patch to just make it into a PGC_POSTMASTER and
> remove the code to support changing it.

I've wondered about that before. But always decided that we wouldn't
gain all that much by making it PGC_POSTMASTER since the effective value
of full_page_writes still needs to be changeable at runtime since we
need to force-enable full page writes during a pg_start/stop_backup().

Greetings,

Andres Freund

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

* Peter Geoghegan (pg(at)heroku(dot)com) wrote:
> I wonder, is anyone really running in production with full_page_writes
> off?

The simple answer to this is "yes, definitely."

I don't know if they change it on-the-fly, but I doubt it.

Thanks,

Stephen

Peter,

> I wonder, is anyone really running in production with full_page_writes
> off?

We're doing so with clients on Joyent (ZFS). And also for "ephemeral"
postgres instances -- ones where we've also turned off fsync.

It's also quite reasonable to assume that future filesystems will also
be able to make better torn page guarantees.

So it should remain a setting. I see no reason why it should be a
SIGHUP setting, though, if there's any code maintenance required to
support it -- frankly, I only change this setting at first system
deployment.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Tue, Sep 3, 2013 at 9:23 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> So it should remain a setting.

Sure. I wasn't suggesting deprecating it or anything.

--
Peter Geoghegan

On Sep 3, 2013 6:23 PM, "Josh Berkus" <josh(at)agliodbs(dot)com> wrote:
>
> Peter,
>
>
> > I wonder, is anyone really running in production with full_page_writes
> > off?
>
> We're doing so with clients on Joyent (ZFS). And also for "ephemeral"
> postgres instances -- ones where we've also turned off fsync.
>
> It's also quite reasonable to assume that future filesystems will also
> be able to make better torn page guarantees.
>
> So it should remain a setting. I see no reason why it should be a
> SIGHUP setting, though, if there's any code maintenance required to
> support it -- frankly, I only change this setting at first system
> deployment.
>

This matches my experience as well - people certainly use it, but don't
change it during runtime.

Not having looked at the code, but doesn't pg_start_backup use at least
parts of the same code path? That one will definitely still be able to
modify it at runtime....

/Magnus

On Mon, Sep 2, 2013 at 10:10 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> There is a significant amount of code supporting the changing of
> full_page_writes while the server is running, including an
> XLOG_FPW_CHANGE wal record. But I don't see the use-case; surely almost
> nobody changes this on a running server, because either your filesystem
> guarantees atomic page writes for you or not.

Although this is true, the administrator's estimate of whether that
guarantee is or is not provided might not be as consistent as the
hardware behavior itself. I am generally of the feeling that having
to restart the server to change setting sucks, and while it surely
sucks less for this setting than some, mostly because few people
change this setting in the first place, I'm still not convinced that
this is moving in the right direction.

> I'd like to submit a patch to just make it into a PGC_POSTMASTER and
> remove the code to support changing it.
>
> Then, I intend to write another patch to make the full-page writes for
> checksums honor the full_page_writes setting. That will be easier to
> write once it's a PGC_POSTMASTER.

I don't think I know exactly what this means.

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

On Tue, Sep 3, 2013 at 07:47:22PM +0200, Magnus Hagander wrote:
>
> On Sep 3, 2013 6:23 PM, "Josh Berkus" <josh(at)agliodbs(dot)com> wrote:
> >
> > Peter,
> >
> >
> > > I wonder, is anyone really running in production with full_page_writes
> > > off?
> >
> > We're doing so with clients on Joyent (ZFS). And also for "ephemeral"
> > postgres instances -- ones where we've also turned off fsync.
> >
> > It's also quite reasonable to assume that future filesystems will also
> > be able to make better torn page guarantees.
> >
> > So it should remain a setting. I see no reason why it should be a
> > SIGHUP setting, though, if there's any code maintenance required to
> > support it -- frankly, I only change this setting at first system
> > deployment.
> >
>
> This matches my experience as well - people certainly use it, but don't change
> it during runtime.
>
> Not having looked at the code, but doesn't pg_start_backup use at least parts
> of the same code path? That one will definitely still be able to modify it at
> runtime....

Yes, this was already mentioned in the thread, but didn't get noticed by
a few folks. In summary, no changes needed here.

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

+ It's impossible for everything to be true. +

On Tue, 2013-09-03 at 15:42 -0400, Robert Haas wrote:
> Although this is true, the administrator's estimate of whether that
> guarantee is or is not provided might not be as consistent as the
> hardware behavior itself. I am generally of the feeling that having
> to restart the server to change setting sucks, and while it surely
> sucks less for this setting than some, mostly because few people
> change this setting in the first place, I'm still not convinced that
> this is moving in the right direction.

I think code complexity matters quite a lot. If we can eliminate some
complex code in a complex area, and all we give up is a feature with
essentially no use case, that sounds like we're moving in the right
direction to me.

I suppose some might be using it as a hack when they really just want to
temporarily disable WAL during a load or something. Seems like a blunt
tool though, and I haven't heard of anyone doing that or suggesting it.
And it doesn't store the page hole anyway, so the FPI during a load is
ordinarily quite small.

> > Then, I intend to write another patch to make the full-page writes for
> > checksums honor the full_page_writes setting. That will be easier to
> > write once it's a PGC_POSTMASTER.
>
> I don't think I know exactly what this means.

XLogSaveBufferForHint() calls XLogCheckBuffer() but doesn't also look at
the full page writes setting (like in XLogInsert()). That means, if
checksums are enabled and full_page_writes is off, we'll still write
some full page images for checksums. I'd like to remedy that.

Regards,
Jeff Davis

On 2013-09-04 07:57:15 -0700, Jeff Davis wrote:
> XLogSaveBufferForHint() calls XLogCheckBuffer() but doesn't also look at
> the full page writes setting (like in XLogInsert()). That means, if
> checksums are enabled and full_page_writes is off, we'll still write
> some full page images for checksums. I'd like to remedy that.

I don't think that's really as easy as it sounds without removing the
ability to do base backups with full_page_writes = off. The interlocking
that would require makes things complex...
Personally I'd rather forbid enabling checkpoints in the combination
with full_page_writes = off. That doesn't seem like a good idea to me
and I am far from convinced it's actually going to work in all corner cases.

Greetings,

Andres Freund

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

Jeff Davis <pgsql(at)j-davis(dot)com> writes:
> On Tue, 2013-09-03 at 15:42 -0400, Robert Haas wrote:
>> Although this is true, the administrator's estimate of whether that
>> guarantee is or is not provided might not be as consistent as the
>> hardware behavior itself. I am generally of the feeling that having
>> to restart the server to change setting sucks, and while it surely
>> sucks less for this setting than some, mostly because few people
>> change this setting in the first place, I'm still not convinced that
>> this is moving in the right direction.

> I think code complexity matters quite a lot. If we can eliminate some
> complex code in a complex area, and all we give up is a feature with
> essentially no use case, that sounds like we're moving in the right
> direction to me.

Isn't this whole discussion academic in view of Andres' point?
http://www.postgresql.org/message-id/20130903121935.GB5783@awork2.anarazel.de

regards, tom lane

On Wed, 2013-09-04 at 17:00 +0200, Andres Freund wrote:
> On 2013-09-04 07:57:15 -0700, Jeff Davis wrote:
> > XLogSaveBufferForHint() calls XLogCheckBuffer() but doesn't also look at
> > the full page writes setting (like in XLogInsert()). That means, if
> > checksums are enabled and full_page_writes is off, we'll still write
> > some full page images for checksums. I'd like to remedy that.
>
> I don't think that's really as easy as it sounds without removing the
> ability to do base backups with full_page_writes = off. The interlocking
> that would require makes things complex...

I didn't dig into that part yet. I was mostly distracted by the code to
support changing full_page_writes with SIGHUP.

One option would be to have XLogInsert return early if full_page_writes
is off, it's an XLOG_FPI record, and forcePageWrites is off.

> Personally I'd rather forbid enabling checkpoints in the combination
> with full_page_writes = off. That doesn't seem like a good idea to me
> and I am far from convinced it's actually going to work in all corner cases.

Hmm. It's good to be cautious when deploying on a less-common
configuration. However, I don't think it's a good idea to reject
seemingly valid combinations that are supposed to work due to a lack of
confidence in the review/testing process.

Might be an area warranting some further review and testing; I'll take a
look, but feel free to tell me if you can think of specific problem
areas.

Regards,
Jeff Davis

On Wed, 2013-09-04 at 11:32 -0400, Tom Lane wrote:
> Jeff Davis <pgsql(at)j-davis(dot)com> writes:
> > I think code complexity matters quite a lot. If we can eliminate some
> > complex code in a complex area, and all we give up is a feature with
> > essentially no use case, that sounds like we're moving in the right
> > direction to me.
>
> Isn't this whole discussion academic in view of Andres' point?

Maybe "complex code" was an overstatement. We'd be able to eliminate the
XLOG_FPW_CHANGE, UpdateFullPageWrites(), and one of the members of
XLogCtlInsert; and make xlog.c slightly shorter in the process.

The first time I looked at doing the patch to honor full_page_writes=off
when checksums are on, the fact that fullPageWrites was changeable was a
distraction. Since I saw little or no value in what the code offered, my
instinct was to see if we could get rid of it.

It looks like Simon went to significant effort to maintain the
full_page_writes as a PGC_SUGHUP:

http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=8366c780

Maybe he has the best perspective on the value versus complexity?

Regards,
Jeff Davis

On 2013-09-04 09:23:20 -0700, Jeff Davis wrote:
> On Wed, 2013-09-04 at 11:32 -0400, Tom Lane wrote:
> > Jeff Davis <pgsql(at)j-davis(dot)com> writes:
> > > I think code complexity matters quite a lot. If we can eliminate some
> > > complex code in a complex area, and all we give up is a feature with
> > > essentially no use case, that sounds like we're moving in the right
> > > direction to me.
> >
> > Isn't this whole discussion academic in view of Andres' point?
>
> Maybe "complex code" was an overstatement. We'd be able to eliminate the
> XLOG_FPW_CHANGE, UpdateFullPageWrites(), and one of the members of
> XLogCtlInsert; and make xlog.c slightly shorter in the process.

That path is also executed during a normal restart and during
promotion. Check the invocation of UpdateFullPageWrites() in
StartupXLOG(). Note that a standby needs to be able to follow a
primaries full_page_writes setting during a promotion.

Greetings,

Andres Freund

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

On Thu, Sep 5, 2013 at 1:55 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-09-04 09:23:20 -0700, Jeff Davis wrote:
>> On Wed, 2013-09-04 at 11:32 -0400, Tom Lane wrote:
>> > Jeff Davis <pgsql(at)j-davis(dot)com> writes:
>> > > I think code complexity matters quite a lot. If we can eliminate some
>> > > complex code in a complex area, and all we give up is a feature with
>> > > essentially no use case, that sounds like we're moving in the right
>> > > direction to me.
>> >
>> > Isn't this whole discussion academic in view of Andres' point?
>>
>> Maybe "complex code" was an overstatement. We'd be able to eliminate the
>> XLOG_FPW_CHANGE, UpdateFullPageWrites(), and one of the members of
>> XLogCtlInsert; and make xlog.c slightly shorter in the process.
>
> That path is also executed during a normal restart and during
> promotion. Check the invocation of UpdateFullPageWrites() in
> StartupXLOG(). Note that a standby needs to be able to follow a
> primaries full_page_writes setting during a promotion.

Yes, this is required for the backup from the standby.

If we make the GUC contect to PGC_POSTMASTER, I think that
we can remove XLOG_FPW_CHANGE and treat full_page_writes
the same way as wal_level, max_connections, i.e., the parameter
which CheckRequiredParameterValues() handles.

Regards,

--
Fujii Masao