Thread: patch: prevent user from setting wal_buffers over 2GB bytes
Hackers, In guc.c, the maximum for wal_buffers is INT_MAX. However, wal_buffers is actually measured in 8KB buffers, not in bytes. This means that users are able to set wal_buffers > 2GB. When the database is started, this can cause a core dump if the WAL offset is > 2GB. Attached patch fixes this issue by lowering the maximum for wal_buffers: josh@Radegast:~/pg94a$ bin/pg_ctl -D data start server starting josh@Radegast:~/pg94a$ LOG: 393216 is outside the valid range for parameter "wal_buffers" (-1 .. 262143) FATAL: configuration file "/home/josh/pg94a/data/postgresql.conf" contains errors Thanks to Andrew Gierth for diagnosing this issue. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
Attachment
On Thu, Jul 30, 2015 at 9:17 PM, Josh Berkus <josh@agliodbs.com> wrote: > In guc.c, the maximum for wal_buffers is INT_MAX. However, wal_buffers > is actually measured in 8KB buffers, not in bytes. This means that > users are able to set wal_buffers > 2GB. When the database is started, > this can cause a core dump if the WAL offset is > 2GB. Why does this cause a core dump? We could consider fixing whatever the problem is rather than capping the value. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 07/31/2015 10:43 AM, Robert Haas wrote: > On Thu, Jul 30, 2015 at 9:17 PM, Josh Berkus <josh@agliodbs.com> wrote: >> In guc.c, the maximum for wal_buffers is INT_MAX. However, wal_buffers >> is actually measured in 8KB buffers, not in bytes. This means that >> users are able to set wal_buffers > 2GB. When the database is started, >> this can cause a core dump if the WAL offset is > 2GB. > > Why does this cause a core dump? We could consider fixing whatever > the problem is rather than capping the value. The underlying issue is that byte position in wal_buffers is a 32-bit INT, so as soon as you exceed that, core dump. Given that useful ranges for wal_buffers are in the 8MB to 128MB range, I don't see that a cap at 2GB is much of a burden. For that reason, I'd prefer capping the value to replumbing. We have not previously documented the limit for this parameter, which is probably how the bug happened in the first place. Clearly no user has been setting it above 2GB, or they would have been core dumping. Oh, and yes, I think this should be backported; this issue exists in all supported versions. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On Fri, Jul 31, 2015 at 8:09 PM, Josh Berkus <josh@agliodbs.com> wrote: > On 07/31/2015 10:43 AM, Robert Haas wrote: >> On Thu, Jul 30, 2015 at 9:17 PM, Josh Berkus <josh@agliodbs.com> wrote: >>> In guc.c, the maximum for wal_buffers is INT_MAX. However, wal_buffers >>> is actually measured in 8KB buffers, not in bytes. This means that >>> users are able to set wal_buffers > 2GB. When the database is started, >>> this can cause a core dump if the WAL offset is > 2GB. >> >> Why does this cause a core dump? We could consider fixing whatever >> the problem is rather than capping the value. > > The underlying issue is that byte position in wal_buffers is a 32-bit > INT, so as soon as you exceed that, core dump. OK. So capping it sounds like the right approach, then. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> >> Why does this cause a core dump? We could consider fixing whatever > >> the problem is rather than capping the value. As far as I experiment with my own evaluation environment using PostgreSQL-9.4.4 on a x86_64 Linux, this problem can be fixed with the patch attached. I have confirmed that applying the patch makes 'wal_buffers = 4GB' works fine, while original PostgreSQL-9.4.4 results in core dump (segfault). I'll be happy if anyone reconfirm this. -- Takashi Horikawa NEC Corporation Knowledge Discovery Research Laboratories > -----Original Message----- > From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Robert Haas > Sent: Tuesday, August 04, 2015 2:29 AM > To: Josh Berkus > Cc: PostgreSQL-development > Subject: Re: [HACKERS] patch: prevent user from setting wal_buffers over > 2GB bytes > > On Fri, Jul 31, 2015 at 8:09 PM, Josh Berkus <josh@agliodbs.com> wrote: > > On 07/31/2015 10:43 AM, Robert Haas wrote: > >> On Thu, Jul 30, 2015 at 9:17 PM, Josh Berkus <josh@agliodbs.com> wrote: > >>> In guc.c, the maximum for wal_buffers is INT_MAX. However, > >>> wal_buffers is actually measured in 8KB buffers, not in bytes. This > >>> means that users are able to set wal_buffers > 2GB. When the > >>> database is started, this can cause a core dump if the WAL offset is > > 2GB. > >> > >> Why does this cause a core dump? We could consider fixing whatever > >> the problem is rather than capping the value. > > > > The underlying issue is that byte position in wal_buffers is a 32-bit > > INT, so as soon as you exceed that, core dump. > > OK. So capping it sounds like the right approach, then. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL > Company > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Takashi Horikawa <t-horikawa@aj.jp.nec.com> writes:
>>>> Why does this cause a core dump? We could consider fixing whatever
>>>> the problem is rather than capping the value.
> As far as I experiment with my own evaluation environment using
> PostgreSQL-9.4.4 on a x86_64 Linux, this problem can be fixed with the patch
> attached.
I'm unsure whether this represents a complete fix ... but even if it does,
it would be awfully easy to re-introduce similar bugs in future code
changes, and who would notice? Josh's approach of restricting the buffer
size seems a lot more robust.
If there were any practical use-case for such large WAL buffers then it
might be worth spending some effort/risk here. But AFAICS, there is not.
Indeed, capping wal_buffers might be argued to be a good thing in itself
because it would prevent users from wasting shared memory foolishly.
So my vote is for the original approach. (I've not read Josh's patch,
so there might be something wrong with it in detail, but I like the
basic approach.)
regards, tom lane
On 2015-08-04 09:49:58 -0400, Tom Lane wrote: > Takashi Horikawa <t-horikawa@aj.jp.nec.com> writes: > >>>> Why does this cause a core dump? We could consider fixing whatever > >>>> the problem is rather than capping the value. > > > As far as I experiment with my own evaluation environment using > > PostgreSQL-9.4.4 on a x86_64 Linux, this problem can be fixed with the patch > > attached. > > I'm unsure whether this represents a complete fix ... but even if it does, > it would be awfully easy to re-introduce similar bugs in future code > changes, and who would notice? Josh's approach of restricting the buffer > size seems a lot more robust. > > If there were any practical use-case for such large WAL buffers then it > might be worth spending some effort/risk here. But AFAICS, there is not. > Indeed, capping wal_buffers might be argued to be a good thing in itself > because it would prevent users from wasting shared memory foolishly. > > So my vote is for the original approach. (I've not read Josh's patch, > so there might be something wrong with it in detail, but I like the > basic approach.) +1
On Tue, Aug 4, 2015 at 9:52 AM, Andres Freund <andres@anarazel.de> wrote: >> So my vote is for the original approach. (I've not read Josh's patch, >> so there might be something wrong with it in detail, but I like the >> basic approach.) > > +1 OK, committed and back-patched that all the way back to 9.0. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> ... Josh's approach of restricting the buffer size seems > a lot more robust. I understand that the capping of approach of restricting the buffer size is much more robust and is suitable in this case. I, howerver, think that the chane from 'page = &XLogCtl->pages[firstIdx * XLOG_BLCKSZ];' to 'page = &XLogCtl->pages[firstIdx * (Size) XLOG_BLCKSZ];' is no harm even when restricting the wal buffer size. It is in harmony with the usage of 'XLogCtl->pages' found in, for example, 'cachedPos = XLogCtl->pages + idx * (Size) XLOG_BLCKSZ;' in GetXLogBuffer(XLogRecPtr ptr) and 'NewPage = (XLogPageHeader) (XLogCtl->pages + nextidx * (Size) XLOG_BLCKSZ); ' in AdvanceXLInsertBuffer(XLogRecPtr upto, bool opportunistic) , etc. Only exception is 'page = &XLogCtl->pages[firstIdx * XLOG_BLCKSZ];' in StartupXLOG(void) -- Takashi Horikawa NEC Corporation Knowledge Discovery Research Laboratories > -----Original Message----- > From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Tom Lane > Sent: Tuesday, August 04, 2015 10:50 PM > To: Horikawa Takashi(堀川 隆) > Cc: PostgreSQL-development > Subject: Re: [HACKERS] patch: prevent user from setting wal_buffers over > 2GB bytes > > Takashi Horikawa <t-horikawa@aj.jp.nec.com> writes: > >>>> Why does this cause a core dump? We could consider fixing whatever > >>>> the problem is rather than capping the value. > > > As far as I experiment with my own evaluation environment using > > PostgreSQL-9.4.4 on a x86_64 Linux, this problem can be fixed with the > > patch attached. > > I'm unsure whether this represents a complete fix ... but even if it does, > it would be awfully easy to re-introduce similar bugs in future code changes, > and who would notice? Josh's approach of restricting the buffer size seems > a lot more robust. > > If there were any practical use-case for such large WAL buffers then it > might be worth spending some effort/risk here. But AFAICS, there is not. > Indeed, capping wal_buffers might be argued to be a good thing in itself > because it would prevent users from wasting shared memory foolishly. > > So my vote is for the original approach. (I've not read Josh's patch, so > there might be something wrong with it in detail, but I like the basic > approach.) > > regards, tom lane > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make > changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers