Re: Why does WAL_DEBUG macro need to be defined by default?

Hi,

I found that by default WAL_DEBUG macro has been defined in
9.2dev and 9.1. I'm very surprised at this. Why does WAL_DEBUG
need to be defined by default? The performance overhead
introduced by WAL_DEBUG is really vanishingly low?

WAL_DEBUG was defined in the following commit:
53dbc27c62d8e1b6c5253feba04a5094cb8fe046

----------------------
Support unlogged tables.

The contents of an unlogged table are WAL-logged; thus, they are not
available on standby servers and are truncated whenever the database
system enters recovery. Indexes on unlogged tables are also unlogged.
Unlogged GiST indexes are not currently supported.
----------------------

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On 07.10.2011 12:19, Fujii Masao wrote:
> Hi,
>
> I found that by default WAL_DEBUG macro has been defined in
> 9.2dev and 9.1. I'm very surprised at this. Why does WAL_DEBUG
> need to be defined by default? The performance overhead
> introduced by WAL_DEBUG is really vanishingly low?
>
> WAL_DEBUG was defined in the following commit:
> 53dbc27c62d8e1b6c5253feba04a5094cb8fe046
>
> ----------------------
> Support unlogged tables.
>
> The contents of an unlogged table are WAL-logged; thus, they are not
> available on standby servers and are truncated whenever the database
> system enters recovery. Indexes on unlogged tables are also unlogged.
> Unlogged GiST indexes are not currently supported.
> ----------------------

I'm pretty sure that change was included in the commit by accident.

That said, the overhead of WAL_DEBUG probably is insignificant, as long
as you don't actually set wal_debug=on. I wonder if we should leave it
enabled.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Fri, Oct 7, 2011 at 5:19 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> I found that by default WAL_DEBUG macro has been defined in
> 9.2dev and 9.1. I'm very surprised at this. Why does WAL_DEBUG
> need to be defined by default? The performance overhead
> introduced by WAL_DEBUG is really vanishingly low?
>
> WAL_DEBUG was defined in the following commit:
> 53dbc27c62d8e1b6c5253feba04a5094cb8fe046
>
> ----------------------
>    Support unlogged tables.
>
>    The contents of an unlogged table are WAL-logged; thus, they are not
>    available on standby servers and are truncated whenever the database
>    system enters recovery.  Indexes on unlogged tables are also unlogged.
>    Unlogged GiST indexes are not currently supported.
> ----------------------

Oh, dear. That was a mistake on my part. :-(

The funny thing is that I've been thinking all of these months about
how convenient it is that we defined WAL_DEBUG in debug builds, not
realizing that (1) we were defining it all the time, not just in debug
builds and (2) I was the one who accidentally did that.

Sorry, all.

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

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> The funny thing is that I've been thinking all of these months
> about how convenient it is that we defined WAL_DEBUG in debug
> builds

IMO, --enable-debug should not do anything but include debugging
symbols. The ability to get a useful stack trace from a production
crash, without compromising performance, is just too important by
itself to consider conditioning any other behavior on it.

-Kevin

On Fri, Oct 7, 2011 at 1:03 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> The funny thing is that I've been thinking all of these months
>> about how convenient it is that we defined WAL_DEBUG in debug
>> builds
>
> IMO, --enable-debug should not do anything but include debugging
> symbols.  The ability to get a useful stack trace from a production
> crash, without compromising performance, is just too important by
> itself to consider conditioning any other behavior on it.

So, should I go revert this change in head and 9.1, or does anyone
else want to argue for Heikki's position that we should just leave it
on, on the theory that it's too cheap to matter?

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

Robert Haas wrote:
> On Fri, Oct 7, 2011 at 1:03 PM, Kevin Grittner
> <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >> The funny thing is that I've been thinking all of these months
> >> about how convenient it is that we defined WAL_DEBUG in debug
> >> builds
> >
> > IMO, --enable-debug should not do anything but include debugging
> > symbols. ?The ability to get a useful stack trace from a production
> > crash, without compromising performance, is just too important by
> > itself to consider conditioning any other behavior on it.
>
> So, should I go revert this change in head and 9.1, or does anyone
> else want to argue for Heikki's position that we should just leave it
> on, on the theory that it's too cheap to matter?

I would just fix it in head.

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

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

On Fri, Oct 7, 2011 at 4:06 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> On Fri, Oct 7, 2011 at 1:03 PM, Kevin Grittner
>> <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
>> > Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> >> The funny thing is that I've been thinking all of these months
>> >> about how convenient it is that we defined WAL_DEBUG in debug
>> >> builds
>> >
>> > IMO, --enable-debug should not do anything but include debugging
>> > symbols. ?The ability to get a useful stack trace from a production
>> > crash, without compromising performance, is just too important by
>> > itself to consider conditioning any other behavior on it.
>>
>> So, should I go revert this change in head and 9.1, or does anyone
>> else want to argue for Heikki's position that we should just leave it
>> on, on the theory that it's too cheap to matter?
>
> I would just fix it in head.

That just seems weird. Either it's cheap enough not to matter (in
which case there's no reason to revert that change at all) or it's
expensive enough to matter (in which case presumably we don't want to
leave it on in 9.1 for the 5 years or so it remains a supported
release).

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

Robert Haas wrote:
> On Fri, Oct 7, 2011 at 4:06 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Robert Haas wrote:
> >> On Fri, Oct 7, 2011 at 1:03 PM, Kevin Grittner
> >> <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> >> > Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >> >> The funny thing is that I've been thinking all of these months
> >> >> about how convenient it is that we defined WAL_DEBUG in debug
> >> >> builds
> >> >
> >> > IMO, --enable-debug should not do anything but include debugging
> >> > symbols. ?The ability to get a useful stack trace from a production
> >> > crash, without compromising performance, is just too important by
> >> > itself to consider conditioning any other behavior on it.
> >>
> >> So, should I go revert this change in head and 9.1, or does anyone
> >> else want to argue for Heikki's position that we should just leave it
> >> on, on the theory that it's too cheap to matter?
> >
> > I would just fix it in head.
>
> That just seems weird. Either it's cheap enough not to matter (in
> which case there's no reason to revert that change at all) or it's
> expensive enough to matter (in which case presumably we don't want to
> leave it on in 9.1 for the 5 years or so it remains a supported
> release).

I am concerned about changing behavior on people in a minor release ---
it is not about risk in this case.

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

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

On Fri, Oct 7, 2011 at 9:59 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> On Fri, Oct 7, 2011 at 4:06 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> > Robert Haas wrote:
>> >> On Fri, Oct 7, 2011 at 1:03 PM, Kevin Grittner
>> >> <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
>> >> > Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> >> >> The funny thing is that I've been thinking all of these months
>> >> >> about how convenient it is that we defined WAL_DEBUG in debug
>> >> >> builds
>> >> >
>> >> > IMO, --enable-debug should not do anything but include debugging
>> >> > symbols. ?The ability to get a useful stack trace from a production
>> >> > crash, without compromising performance, is just too important by
>> >> > itself to consider conditioning any other behavior on it.
>> >>
>> >> So, should I go revert this change in head and 9.1, or does anyone
>> >> else want to argue for Heikki's position that we should just leave it
>> >> on, on the theory that it's too cheap to matter?
>> >
>> > I would just fix it in head.
>>
>> That just seems weird.  Either it's cheap enough not to matter (in
>> which case there's no reason to revert that change at all) or it's
>> expensive enough to matter (in which case presumably we don't want to
>> leave it on in 9.1 for the 5 years or so it remains a supported
>> release).
>
> I am concerned about changing behavior on people in a minor release ---
> it is not about risk in this case.

Well, I still maintain that if the performance impact is low enough
that we can get away with that, it's probably not worth fixing in
master either. But at any rate, we now have three opinions on what to
do about this. Anyone else want to cast a vote (preferably not for an
entirely new option)?

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Oct 7, 2011 at 4:06 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> I would just fix it in head.

> That just seems weird. Either it's cheap enough not to matter (in
> which case there's no reason to revert that change at all) or it's
> expensive enough to matter (in which case presumably we don't want to
> leave it on in 9.1 for the 5 years or so it remains a supported
> release).

It needs to be reverted. I don't understand why you didn't do that
instantly upon the mistake being pointed out to you.

regards, tom lane