Re: Logging WAL when updating hintbit

Hi all,

I attached patch adds new wal_level 'all'.
If wal_level is set 'all', the server logs WAL not only when wal_level
is set 'hot_standby' ,but also when updating hint bit.
That is, we will be able to know all of the changed block number by
reading the WALs.
This wal_level is infrastructure for fast failback. (i.g., without fresh backup)
It need to cooperate with pg_rewind.

Not only that, I think it will be profitable infrastructure for
differential backup.
And it leads to improve performance at standby server side. Because
the standby server doesn't update hintbit by itself, but FPW is
replicated to standby server and applied.

It is very simple patch, server writes FPW at same timing as when
checksum is enabled. i.g., just without calculate checksum.

Discussion of Fast failback is here
<http://www.postgresql.org/message-id/CAF8Q-Gy7xa60HwXc0MKajjkWFEbFDWTG=gGyu1KmT+s2xcQ-bw@mail.gmail.com>

Regards,

-------
Sawada Masahiko

On Thu, Nov 14, 2013 at 3:02 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> I attached patch adds new wal_level 'all'.
> If wal_level is set 'all', the server logs WAL not only when wal_level
> is set 'hot_standby' ,but also when updating hint bit.
> That is, we will be able to know all of the changed block number by
> reading the WALs.
Is 'all' a name really suited? It looks too general. I don't have a
name on top of my mind but what about something like full_pages or
something similar...

> This wal_level is infrastructure for fast failback. (i.g., without fresh backup)
> It need to cooperate with pg_rewind.
I am not sure that using as reason the possible interactions of a
contrib module not in core is a reason sufficient to justify the
presence of a new wal_level, and pg_rewind is still a young solution
that needs to be improved. So such a patch looks premature IMO, but
the idea is interesting and might cover many needs for external
projects.

> Not only that, I think it will be profitable infrastructure for
> differential backup.
Yep, agreed. This might help some projects in this area.

> And it leads to improve performance at standby server side. Because
> the standby server doesn't update hintbit by itself, but FPW is
> replicated to standby server and applied.
It would be interesting to see some numbers here.

This is clearly a WIP patch so it does not matter now, but if you
submit it later on, be sure to add some comments in bufmgr.c as well
as documentation for the new option.
Regards,
--
Michael

On 11/14/2013 07:02 AM, Sawada Masahiko wrote:

> I attached patch adds new wal_level 'all'.

Shouldn't this be a separate setting? It's useful for storage which
requires rewriting a partially written sector before it can be read again.

--
Florian Weimer / Red Hat Product Security Team

On Thu, Nov 14, 2013 at 3:53 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Thu, Nov 14, 2013 at 3:02 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> I attached patch adds new wal_level 'all'.
>> If wal_level is set 'all', the server logs WAL not only when wal_level
>> is set 'hot_standby' ,but also when updating hint bit.
>> That is, we will be able to know all of the changed block number by
>> reading the WALs.
> Is 'all' a name really suited? It looks too general. I don't have a
> name on top of my mind but what about something like full_pages or
> something similar...

Yes, I'm worried about name of value.
But 'full_pages' sounds good for me.

>
>> This wal_level is infrastructure for fast failback. (i.g., without fresh backup)
>> It need to cooperate with pg_rewind.
> I am not sure that using as reason the possible interactions of a
> contrib module not in core is a reason sufficient to justify the
> presence of a new wal_level, and pg_rewind is still a young solution
> that needs to be improved. So such a patch looks premature IMO, but
> the idea is interesting and might cover many needs for external
> projects.
>
>> Not only that, I think it will be profitable infrastructure for
>> differential backup.
> Yep, agreed. This might help some projects in this area.
>
>> And it leads to improve performance at standby server side. Because
>> the standby server doesn't update hintbit by itself, but FPW is
>> replicated to standby server and applied.
> It would be interesting to see some numbers here.

I think this patch provide several benefit for feature. The fast
failback with pg_rewind is one of them.
If I want to provide only the fast failback with pg_rewind, this patch
logs too much information.
Just logging changed block number is enough for that.

As you said pg_rewind is still a young solution. But It very cool and
flexible solution.
I'm going to improve pg_rewind actively.

>
> This is clearly a WIP patch so it does not matter now, but if you
> submit it later on, be sure to add some comments in bufmgr.c as well
> as documentation for the new option.

Yes, will do.

--
Regards,

-------
Sawada Masahiko

On Thu, Nov 14, 2013 at 7:51 PM, Florian Weimer <fweimer(at)redhat(dot)com> wrote:
> On 11/14/2013 07:02 AM, Sawada Masahiko wrote:
>
>> I attached patch adds new wal_level 'all'.
>
>
> Shouldn't this be a separate setting? It's useful for storage which
> requires rewriting a partially written sector before it can be read again.
>

Thank you for comment.
Actually, I had thought to add separate parameter.
If so, this feature logs enough information with all of the wal_level
(e.g., minimal) ?
And I thought that relation between wal_lvel and new feature would be
confuse user.

Regards,

-------
Sawada Masahiko

On 11/14/13, 1:02 AM, Sawada Masahiko wrote:
> I attached patch adds new wal_level 'all'.

Compiler warning:

pg_controldata.c: In function ‘wal_level_str’:
pg_controldata.c:72:2: warning: enumeration value ‘WAL_LEVEL_ALL’ not handled in switch [-Wswitch]

On Fri, Nov 15, 2013 at 11:33 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 11/14/13, 1:02 AM, Sawada Masahiko wrote:
>> I attached patch adds new wal_level 'all'.
>
> Compiler warning:
>
> pg_controldata.c: In function ‘wal_level_str’:
> pg_controldata.c:72:2: warning: enumeration value ‘WAL_LEVEL_ALL’ not handled in switch [-Wswitch]
>

Thank you for report!
I have fixed it.

--
Regards,

-------
Sawada Masahiko

(2013/11/15 19:27), Sawada Masahiko wrote:
> On Thu, Nov 14, 2013 at 7:51 PM, Florian Weimer <fweimer(at)redhat(dot)com> wrote:
>> On 11/14/2013 07:02 AM, Sawada Masahiko wrote:
>>
>>> I attached patch adds new wal_level 'all'.
>>
>>
>> Shouldn't this be a separate setting? It's useful for storage which
>> requires rewriting a partially written sector before it can be read again.
>>
>
> Thank you for comment.
> Actually, I had thought to add separate parameter.
I think that he said that if you can proof that amount of WAL is almost same and
without less performance same as before, you might not need to separate
parameter in your patch.

Did you test about amount of WAL size in your patch?
I'd like to know it.

Regards,
--
Mitsumasa KONDO
NTT Open Source Software Center

On Thu, Nov 14, 2013 at 1:02 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> I attached patch adds new wal_level 'all'.
> If wal_level is set 'all', the server logs WAL not only when wal_level
> is set 'hot_standby' ,but also when updating hint bit.
> That is, we will be able to know all of the changed block number by
> reading the WALs.
> This wal_level is infrastructure for fast failback. (i.g., without fresh backup)
> It need to cooperate with pg_rewind.

This needs to be a separate parameter, not something that gets jammed
into wal_level.

I'm also not 100% sure we want it, but let's hear what others think.

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

On Tue, Nov 19, 2013 at 3:54 PM, KONDO Mitsumasa
<kondo(dot)mitsumasa(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> (2013/11/15 19:27), Sawada Masahiko wrote:
>>
>> On Thu, Nov 14, 2013 at 7:51 PM, Florian Weimer <fweimer(at)redhat(dot)com>
>> wrote:
>>>
>>> On 11/14/2013 07:02 AM, Sawada Masahiko wrote:
>>>
>>>> I attached patch adds new wal_level 'all'.
>>>
>>>
>>>
>>> Shouldn't this be a separate setting? It's useful for storage which
>>> requires rewriting a partially written sector before it can be read
>>> again.
>>>
>>
>> Thank you for comment.
>> Actually, I had thought to add separate parameter.
>
> I think that he said that if you can proof that amount of WAL is almost same
> and
> without less performance same as before, you might not need to separate
> parameter in your patch.
>

Thanks!
I took it wrong.
I think that there are quite a few difference amount of WAL.

> Did you test about amount of WAL size in your patch?

Not yet. I will do that.

Regards,

-------
Sawada Masahiko

On 19 November 2013 22:19, Sawada Masahiko Wrote

> >>
> >> Thank you for comment.
> >> Actually, I had thought to add separate parameter.
> >
> > I think that he said that if you can proof that amount of WAL is
> > almost same and without less performance same as before, you might
> not
> > need to separate parameter in your patch.
> >
>
> Thanks!
> I took it wrong.
> I think that there are quite a few difference amount of WAL.
>
> > Did you test about amount of WAL size in your patch?
>
> Not yet. I will do that.

1. Patch applies cleanly to master HEAD.
2. No Compilation Warning.
3. It works as per the patch expectation.

Some Suggestion:
1. Add new WAL level ("all") in comment in postgresql.conf
wal_level = hot_standby # minimal, archive, or hot_standby

Performance Test Result:
Run with pgbench for 300 seconds

WAL level : hot_standby
WAL Size : 111BF8A8
TPS : 125

WAL level : all
WAL Size : 11DB5AF8
TPS : 122

* TPS is almost constant but WAL size is increased around 11M.

This is the first level of observation, I will continue to test few more scenarios including performance test on standby.

Regards,
Dilip Kumar

On Wed, Nov 20, 2013 at 9:19 PM, Dilip kumar <dilip(dot)kumar(at)huawei(dot)com> wrote:
> On 19 November 2013 22:19, Sawada Masahiko Wrote
>>> >
>>
>> Thanks!
>> I took it wrong.
>> I think that there are quite a few difference amount of WAL.
>>
>> > Did you test about amount of WAL size in your patch?
>>
>> Not yet. I will do that.
>
> 1. Patch applies cleanly to master HEAD.
> 2. No Compilation Warning.
> 3. It works as per the patch expectation.
>
> Some Suggestion:
> 1. Add new WAL level ("all") in comment in postgresql.conf
> wal_level = hot_standby # minimal, archive, or hot_standby

Thank you for reviewing the patch.
Yes, I will do that. And I'm going to implement documentation patch.

>
> Performance Test Result:
> Run with pgbench for 300 seconds
>
> WAL level : hot_standby
> WAL Size : 111BF8A8
> TPS : 125
>
> WAL level : all
> WAL Size : 11DB5AF8
> TPS : 122
>
> * TPS is almost constant but WAL size is increased around 11M.
>
> This is the first level of observation, I will continue to test few more scenarios including performance test on standby.

Thank you for performance testing.
According of test result, TPS of 'all' lower than 'hot_standby',but
WAL size is increased.
I think that it should be separate parameter.
And TPS on master side is is almost constant. so this patch might have
several benefit for performance
improvement on standby side if the result of performance test on
standby side is improved.

Regards,

-------
Sawada Masahiko

On 20 November 2013 22:12, Sawada Masahiko Wrote

> >
> > 1. Patch applies cleanly to master HEAD.
> > 2. No Compilation Warning.
> > 3. It works as per the patch expectation.
> >
> > Some Suggestion:
> > 1. Add new WAL level ("all") in comment in postgresql.conf
> > wal_level = hot_standby # minimal, archive,
> or hot_standby
>
> Thank you for reviewing the patch.
> Yes, I will do that. And I'm going to implement documentation patch.

OK, once I get it, I will review the same.

> >
> > Performance Test Result:
> > Run with pgbench for 300 seconds
> >
> > WAL level : hot_standby
> > WAL Size : 111BF8A8
> > TPS : 125
> >
> > WAL level : all
> > WAL Size : 11DB5AF8
> > TPS : 122
> >
> > * TPS is almost constant but WAL size is increased around 11M.
> >
> > This is the first level of observation, I will continue to test few
> more scenarios including performance test on standby.
>
> Thank you for performance testing.
> According of test result, TPS of 'all' lower than 'hot_standby',but
> WAL size is increased.
> I think that it should be separate parameter.
> And TPS on master side is is almost constant. so this patch might have
> several benefit for performance improvement on standby side if the
> result of performance test on standby side is improved.

[Performance test on standby:]

I have executed pgbench on master with WAL LEVEL "hot_stanby" and "all" option, and after that run pgbench on standby with "select-only" option.

WAL LEVEL (on master) : hot_standby
Select TPS (on standby) : 4098

WAL LEVEL (on master) : all
Select TPS (on standby) : 4115

Regards,
Dilip

On Thu, Nov 21, 2013 at 8:59 PM, Dilip kumar <dilip(dot)kumar(at)huawei(dot)com> wrote:
> On 20 November 2013 22:12, Sawada Masahiko Wrote
>
>> >
>> > 1. Patch applies cleanly to master HEAD.
>> > 2. No Compilation Warning.
>> > 3. It works as per the patch expectation.
>> >
>> > Some Suggestion:
>> > 1. Add new WAL level ("all") in comment in postgresql.conf
>> > wal_level = hot_standby # minimal, archive,
>> or hot_standby
>>
>> Thank you for reviewing the patch.
>> Yes, I will do that. And I'm going to implement documentation patch.
>
> OK, once I get it, I will review the same.
>
>
>> >
>> > Performance Test Result:
>> > Run with pgbench for 300 seconds
>> >
>> > WAL level : hot_standby
>> > WAL Size : 111BF8A8
>> > TPS : 125
>> >
>> > WAL level : all
>> > WAL Size : 11DB5AF8
>> > TPS : 122
>> >
>> > * TPS is almost constant but WAL size is increased around 11M.
>> >
>> > This is the first level of observation, I will continue to test few
>> more scenarios including performance test on standby.
>>
>> Thank you for performance testing.
>> According of test result, TPS of 'all' lower than 'hot_standby',but
>> WAL size is increased.
>> I think that it should be separate parameter.
>> And TPS on master side is is almost constant. so this patch might have
>> several benefit for performance improvement on standby side if the
>> result of performance test on standby side is improved.
>
> [Performance test on standby:]
>
> I have executed pgbench on master with WAL LEVEL "hot_stanby" and "all" option, and after that run pgbench on standby with "select-only" option.
>
> WAL LEVEL (on master) : hot_standby
> Select TPS (on standby) : 4098
>
> WAL LEVEL (on master) : all
> Select TPS (on standby) : 4115
>

Thank you for testing!

It seems to have a little benefit for performance improvement on standby side.
It need to more test to write such thing into the documentation patch.

And I'm going to implement the patch as separated parameter now.
I think that this parameter should allow to use something together
with 'archive' and 'hot_standby'.
IWO not allow with 'minimal'.
Thought?

Regards,

-------
Sawada Masahiko

On Tue, 2013-11-19 at 11:42 -0500, Robert Haas wrote:
> On Thu, Nov 14, 2013 at 1:02 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > I attached patch adds new wal_level 'all'.
> > If wal_level is set 'all', the server logs WAL not only when wal_level
> > is set 'hot_standby' ,but also when updating hint bit.
> > That is, we will be able to know all of the changed block number by
> > reading the WALs.
> > This wal_level is infrastructure for fast failback. (i.g., without fresh backup)
> > It need to cooperate with pg_rewind.
>
> I'm also not 100% sure we want it, but let's hear what others think.

My take is that configuration options should be used to serve different
use cases. One thing I like about postgres is that it doesn't have
options for the sake of options.

The trade-off here seems to be: if you want fast failback, you have to
write more WAL during normal operation. But it's not clear to me which
one I should choose for a given installation. If there's a lot of data,
then fast failback is nice, but then again, logging the hint bits on a
large amount of data might be painful during normal operation. The only
time the choice is easy is when you already have checksums enabled.

I think we should work some more in this area first so we can end up
with something that works for everyone; or at least a more clear choice
to offer users. My hope is that it will go something like:

1. We get more reports from the field about checksum performance
2. pg_rewind gets some more attention
3. we find a way to upgrade a replica set using pg_upgrade and pg_rewind
so that the replicas do not all need a new basebackup after an upgrade
4. We further mitigate the performance impact of logging all page
modifications
5. We eventually see that the benefits of logging all page modifications
outweigh the performance cost for most people, and start recommending to
most people
6. The other WAL levels become more specialized for single, unreplicated
instances

That's just a hope, of course, but we've made some progress and I think
it's a plausible outcome. As it stands, the replica re-sync after any
failover or upgrade seems to be a design gap.

All that being said, I don't object to this option -- I just want it to
lead us somewhere. Not be another option that we keep around forever
with conflicting recommendations about how to set it.

Regards,
Jeff Davis

On Sun, Nov 24, 2013 at 6:02 AM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Tue, 2013-11-19 at 11:42 -0500, Robert Haas wrote:
> My take is that configuration options should be used to serve different
> use cases. One thing I like about postgres is that it doesn't have
> options for the sake of options.
>
> The trade-off here seems to be: if you want fast failback, you have to
> write more WAL during normal operation. But it's not clear to me which
> one I should choose for a given installation. If there's a lot of data,
> then fast failback is nice, but then again, logging the hint bits on a
> large amount of data might be painful during normal operation. The only
> time the choice is easy is when you already have checksums enabled.
>
> I think we should work some more in this area first so we can end up
> with something that works for everyone; or at least a more clear choice
> to offer users. My hope is that it will go something like:
>
> 1. We get more reports from the field about checksum performance
> 2. pg_rewind gets some more attention
> 3. we find a way to upgrade a replica set using pg_upgrade and pg_rewind
> so that the replicas do not all need a new basebackup after an upgrade
> 4. We further mitigate the performance impact of logging all page
> modifications
> 5. We eventually see that the benefits of logging all page modifications
> outweigh the performance cost for most people, and start recommending to
> most people
> 6. The other WAL levels become more specialized for single, unreplicated
> instances
>
> That's just a hope, of course, but we've made some progress and I think
> it's a plausible outcome. As it stands, the replica re-sync after any
> failover or upgrade seems to be a design gap.
>
> All that being said, I don't object to this option -- I just want it to
> lead us somewhere. Not be another option that we keep around forever
> with conflicting recommendations about how to set it.
>

Thank you for feedback.

I agree with you.
We need to more report regarding checksum performance first. I will do that.

I attached the new version patch which adds separated parameter
'enable_logging_hintbit'.
It works same as previous patch, just independence from wal_level and
name is changed.
One changed behave is that it doesn't work together with 'minimal' wal_level.

Regards,

-------
Sawada Masahiko

On Mon, Nov 25, 2013 at 9:02 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Sun, Nov 24, 2013 at 6:02 AM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
>> On Tue, 2013-11-19 at 11:42 -0500, Robert Haas wrote:
>> My take is that configuration options should be used to serve different
>> use cases. One thing I like about postgres is that it doesn't have
>> options for the sake of options.
>>
>> The trade-off here seems to be: if you want fast failback, you have to
>> write more WAL during normal operation. But it's not clear to me which
>> one I should choose for a given installation. If there's a lot of data,
>> then fast failback is nice, but then again, logging the hint bits on a
>> large amount of data might be painful during normal operation. The only
>> time the choice is easy is when you already have checksums enabled.
>>
>> I think we should work some more in this area first so we can end up
>> with something that works for everyone; or at least a more clear choice
>> to offer users. My hope is that it will go something like:
>>
>> 1. We get more reports from the field about checksum performance
>> 2. pg_rewind gets some more attention
>> 3. we find a way to upgrade a replica set using pg_upgrade and pg_rewind
>> so that the replicas do not all need a new basebackup after an upgrade
>> 4. We further mitigate the performance impact of logging all page
>> modifications
>> 5. We eventually see that the benefits of logging all page modifications
>> outweigh the performance cost for most people, and start recommending to
>> most people
>> 6. The other WAL levels become more specialized for single, unreplicated
>> instances
>>
>> That's just a hope, of course, but we've made some progress and I think
>> it's a plausible outcome. As it stands, the replica re-sync after any
>> failover or upgrade seems to be a design gap.
>>
>> All that being said, I don't object to this option -- I just want it to
>> lead us somewhere. Not be another option that we keep around forever
>> with conflicting recommendations about how to set it.
>>
>
> Thank you for feedback.
>
> I agree with you.
> We need to more report regarding checksum performance first. I will do that.
>

I did performance test on my environment.
Performance test result:

pgbench -T 600

Plane postgres : 460
Plane postgres with checksum : 450
Logging hintbit : 456

There is not huge performance degradation between three cases.
I think that It is better point that user can change the values
without creating database cluster newly.

Regards,

-------
Sawada Masahiko

On 11/25/13, 7:02 AM, Sawada Masahiko wrote:
> I attached the new version patch which adds separated parameter
> 'enable_logging_hintbit'.

Let's not add more parameters named enable_*. Every parameter enables
something.

Also, I'd be worried about confusion with other log_* and logging_*
parameters, which are about something other than WAL. Maybe it should
be called wal_log_hintbits (or walog_hintbits?).

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 11/25/13, 7:02 AM, Sawada Masahiko wrote:
>> I attached the new version patch which adds separated parameter
>> 'enable_logging_hintbit'.

> Let's not add more parameters named enable_*. Every parameter enables
> something.

More to the point: there's a convention that we use enable_foo
for planner control parameters that gate usage of a "foo" plan type.
This is not in that category.

regards, tom lane

On Wed, Nov 27, 2013 at 2:28 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> On 11/25/13, 7:02 AM, Sawada Masahiko wrote:
>>> I attached the new version patch which adds separated parameter
>>> 'enable_logging_hintbit'.
>
>> Let's not add more parameters named enable_*. Every parameter enables
>> something.
>
> More to the point: there's a convention that we use enable_foo
> for planner control parameters that gate usage of a "foo" plan type.
> This is not in that category.
>

Thank you for feedback.
I attached the v4 patch which have modified. Just name changed to
'wal_log_hintbtis'.
And i'm also implementing documentation patch.

Regards,

-------
Sawada Masahiko

On Wed, Nov 27, 2013 at 11:04 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> Thank you for feedback.
> I attached the v4 patch which have modified. Just name changed to
> 'wal_log_hintbtis'.
Few typos in this patch found while I quickly went through:
1) s/logging hintbit/logging of hint bits/
2) s/hintbit/hint bits/
Except that it looks that you haven't forgotten any code paths in your
patch, so far it looks good.
Regards,
--
Michael

On Wed, Nov 27, 2013 at 11:22 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Nov 27, 2013 at 11:04 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> Thank you for feedback.
>> I attached the v4 patch which have modified. Just name changed to
>> 'wal_log_hintbtis'.
> Few typos in this patch found while I quickly went through:
> 1) s/logging hintbit/logging of hint bits/
> 2) s/hintbit/hint bits/
> Except that it looks that you haven't forgotten any code paths in your
> patch, so far it looks good.

Thank you for reviewing the patch.

I attached new version patch which have modify typos and added
documentation patch.
The documentation part of patch is implemented by Samrat Revagade.

Regards,

-------
Sawada Masahiko

On Thu, Nov 28, 2013 at 9:42 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> I attached new version patch which have modify typos and added
> documentation patch.
> The documentation part of patch is implemented by Samrat Revagade.
Thanks for the new version. The documentation still has some typo:
- is ,<literal>off</> => is <literal>off</>

I have been pondering about this feature over the weekend and I have
to admit that the approach using a GUC that can be modified after
server initialization is not suited. What we want with this feature is
to be able to include hint bits in WAL to perform WAL differential
operations which is in some way what for example pg_rewing is doing by
analyzing the relation blocks modified since the WAL fork point of a
master with one of its promoted slave. But if this parameter can be
modified by user at will, a given server could finish with a set of
WAL files having inconsistent hint bit data (some files might have the
hint bits, others not), which could create corrupted data when they
are replayed in recovery.

Considering that, it would make more sense to have this option
settable with initdb only and not changeable after initialization, in
the same fashion as checksums. Having a GUC that can be used to check
if this option is set or not using a SQL command could be an
additional patch on top of the core feature.

This does not mean of course that this patch has to be completely
reworked as the core part of the patch, the documentation and the
control file part would remain more or less the same.

Regards,
--
Michael

On Mon, Dec 2, 2013 at 12:54 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Thu, Nov 28, 2013 at 9:42 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> I attached new version patch which have modify typos and added
>> documentation patch.
>> The documentation part of patch is implemented by Samrat Revagade.
> Thanks for the new version. The documentation still has some typo:
> - is ,<literal>off</> => is <literal>off</>
>
> I have been pondering about this feature over the weekend and I have
> to admit that the approach using a GUC that can be modified after
> server initialization is not suited. What we want with this feature is
> to be able to include hint bits in WAL to perform WAL differential
> operations which is in some way what for example pg_rewing is doing by
> analyzing the relation blocks modified since the WAL fork point of a
> master with one of its promoted slave. But if this parameter can be
> modified by user at will, a given server could finish with a set of
> WAL files having inconsistent hint bit data (some files might have the
> hint bits, others not), which could create corrupted data when they
> are replayed in recovery.

Yep, that's a problem.

> Considering that, it would make more sense to have this option
> settable with initdb only and not changeable after initialization, in
> the same fashion as checksums. Having a GUC that can be used to check
> if this option is set or not using a SQL command could be an
> additional patch on top of the core feature.
>
> This does not mean of course that this patch has to be completely
> reworked as the core part of the patch, the documentation and the
> control file part would remain more or less the same.

Forcing it to be done only an initdb-time is excessive. I think you
can just make it PGC_POSTMASTER and have it participate in the
XLOG_PARAMETER_CHANGE mechanism. pg_rewind can check that it's set in
the control file before agreeing to rewind. As long as it was set at
the time the master last entered read-write mode (which is what the
XLOG_PARAMETER_CHANGE stuff does) you should be fine, unless of course
I haven't had enough caffeine this morning, which is certainly
possible.

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

On Tue, Dec 3, 2013 at 1:08 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Dec 2, 2013 at 12:54 AM, Michael Paquier
>> Considering that, it would make more sense to have this option
>> settable with initdb only and not changeable after initialization, in
>> the same fashion as checksums. Having a GUC that can be used to check
>> if this option is set or not using a SQL command could be an
>> additional patch on top of the core feature.
>
> Forcing it to be done only an initdb-time is excessive. I think you
> can just make it PGC_POSTMASTER and have it participate in the
> XLOG_PARAMETER_CHANGE mechanism. pg_rewind can check that it's set in
> the control file before agreeing to rewind.
Yes, this is only a matter of adding a couple of lines of code.

> As long as it was set at
> the time the master last entered read-write mode (which is what the
> XLOG_PARAMETER_CHANGE stuff does) you should be fine, unless of course
> I haven't had enough caffeine this morning, which is certainly
> possible.
Indeed, I forgot this code path. Completing XLogReportParameters for
saving the state and xlog_redo for replay would be enough.
--
Michael

On Tue, Dec 3, 2013 at 11:57 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Dec 3, 2013 at 1:08 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> As long as it was set at
>> the time the master last entered read-write mode (which is what the
>> XLOG_PARAMETER_CHANGE stuff does) you should be fine, unless of course
>> I haven't had enough caffeine this morning, which is certainly
>> possible.
> Indeed, I forgot this code path. Completing XLogReportParameters for
> saving the state and xlog_redo for replay would be enough.
Wait a minute, I retract this argument. By using this method a master
server would be able to produce WAL files with inconsistent hint bit
data when they are replayed if log_hint_bits is changed after a
restart of the master.
--
Michael

On Tue, Dec 3, 2013 at 12:02 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Dec 3, 2013 at 11:57 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Tue, Dec 3, 2013 at 1:08 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> Forcing it to be done only an initdb-time is excessive. I think you
>>> can just make it PGC_POSTMASTER and have it participate in the
>>> XLOG_PARAMETER_CHANGE mechanism. pg_rewind can check that it's set in
>>> the control file before agreeing to rewind.

Yep, I will modify it.

>> Indeed, I forgot this code path. Completing XLogReportParameters for
>> saving the state and xlog_redo for replay would be enough.
> Wait a minute, I retract this argument. By using this method a master
> server would be able to produce WAL files with inconsistent hint bit
> data when they are replayed if log_hint_bits is changed after a
> restart of the master.

How case does it occur?
I think pg_rewind can disagree to rewind if log_hint_bits is changed to 'OFF'.
Is this not enough?

Regards,

-------
Sawada Masahiko

On Tue, Dec 3, 2013 at 3:30 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Tue, Dec 3, 2013 at 12:02 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> Indeed, I forgot this code path. Completing for
>>> saving the state and xlog_redo for replay would be enough.
>> Wait a minute, I retract this argument. By using this method a master
>> server would be able to produce WAL files with inconsistent hint bit
>> data when they are replayed if log_hint_bits is changed after a
>> restart of the master.
>
> How case does it occur?
> I think pg_rewind can disagree to rewind if log_hint_bits is changed to 'OFF'.
> Is this not enough?

After more thinking...
Before performing a rewind on a node, what we need to know is that
log_hint_bits was set to true when WAL forked, because of the issue
that Robert mentioned here:
http://www.postgresql.org/message-id/519E5493.5060800@vmware.com
It does not really matter if the node used log_hint_bits set to false
in its latest state (Node to-be-rewinded might have been restarted
after WAL forked).

So, after more thinking, yes using XLOG_PARAMETER_CHANGE and
PGC_POSTMASTER for this parameter would be enough. However on the
pg_rewind side we would need to track the value of log_hint_bits when
analyzing the WALs and ensure that it was set to true at fork point.
This is not something that the core should about though.
--
Michael

On Tue, Dec 3, 2013 at 4:28 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Dec 3, 2013 at 3:30 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> On Tue, Dec 3, 2013 at 12:02 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>> Indeed, I forgot this code path. Completing for
>>>> saving the state and xlog_redo for replay would be enough.
>>> Wait a minute, I retract this argument. By using this method a master
>>> server would be able to produce WAL files with inconsistent hint bit
>>> data when they are replayed if log_hint_bits is changed after a
>>> restart of the master.
>>
>> How case does it occur?
>> I think pg_rewind can disagree to rewind if log_hint_bits is changed to 'OFF'.
>> Is this not enough?
>
> After more thinking...
> Before performing a rewind on a node, what we need to know is that
> log_hint_bits was set to true when WAL forked, because of the issue
> that Robert mentioned here:
> http://www.postgresql.org/message-id/519E5493.5060800@vmware.com
> It does not really matter if the node used log_hint_bits set to false
> in its latest state (Node to-be-rewinded might have been restarted
> after WAL forked).
>
> So, after more thinking, yes using XLOG_PARAMETER_CHANGE and
> PGC_POSTMASTER for this parameter would be enough. However on the
> pg_rewind side we would need to track the value of log_hint_bits when
> analyzing the WALs and ensure that it was set to true at fork point.
> This is not something that the core should about though.

Yep, pg_rewind needs to track the value of wal_log_hintbits.
I think value of wal_log_hintbits always needs to have been set true
after fork point.
And if wal_log_hintbits is set false when we perform pg_rewind, we can not that.

Regards,

-------
Sawada Masahiko

On Tue, Dec 3, 2013 at 5:34 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Tue, Dec 3, 2013 at 4:28 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Tue, Dec 3, 2013 at 3:30 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>
>> After more thinking...
>> Before performing a rewind on a node, what we need to know is that
>> log_hint_bits was set to true when WAL forked, because of the issue
>> that Robert mentioned here:
>> http://www.postgresql.org/message-id/519E5493.5060800@vmware.com
>> It does not really matter if the node used log_hint_bits set to false
>> in its latest state (Node to-be-rewinded might have been restarted
>> after WAL forked).
>>
>> So, after more thinking, yes using XLOG_PARAMETER_CHANGE and
>> PGC_POSTMASTER for this parameter would be enough. However on the
>> pg_rewind side we would need to track the value of log_hint_bits when
>> analyzing the WALs and ensure that it was set to true at fork point.
>> This is not something that the core should about though.
>
> Yep, pg_rewind needs to track the value of wal_log_hintbits.
> I think value of wal_log_hintbits always needs to have been set true
> after fork point.
> And if wal_log_hintbits is set false when we perform pg_rewind, we can not that.
>

I attached the patch which have modified based on Robert suggestion,
and fixed typo.

Regards,

-------
Sawada Masahiko

On 04 December 2013, Sawada Masahiko Wrote

> I attached the patch which have modified based on Robert suggestion,
> and fixed typo.

I have reviewed the modified patch and I have some comments..

1. Patch need to be rebased (failed applying on head)

2. crc field should be at end in ControlFileData struct, because for crc calculation, it directly take the offset of crc field in ControlFileData.

/* CRC of all above ... MUST BE LAST! */
pg_crc32 crc;
+
+ /* Enable logging WAL when updating hint bits */
+ bool wal_log_hintbits;
} ControlFileData;

3. wal_log_hintbits field should be printed in PrintControlValues function.

Regards,
Dilip

On Fri, Dec 13, 2013 at 1:51 PM, Dilip kumar <dilip(dot)kumar(at)huawei(dot)com> wrote:
> On 04 December 2013, Sawada Masahiko Wrote
>
>
>> I attached the patch which have modified based on Robert suggestion,
>> and fixed typo.
>
> I have reviewed the modified patch and I have some comments..
>
> 1. Patch need to be rebased (failed applying on head)
>
> 2. crc field should be at end in ControlFileData struct, because for crc calculation, it directly take the offset of crc field in ControlFileData.
>
> /* CRC of all above ... MUST BE LAST! */
> pg_crc32 crc;
> +
> + /* Enable logging WAL when updating hint bits */
> + bool wal_log_hintbits;
> } ControlFileData;
>
> 3. wal_log_hintbits field should be printed in PrintControlValues function.
>

Thank you for reviewing the patch!
I have modified the patch base on your comment, and I attached the v7 patch.

Regards,

-------
Sawada Masahiko

On Fri, Dec 13, 2013 at 11:25, Sawada Masahiko Wrote

> >> I attached the patch which have modified based on Robert suggestion,
> >> and fixed typo.
> >
> > I have reviewed the modified patch and I have some comments..
> >
> > 1. Patch need to be rebased (failed applying on head)
> >
> > 2. crc field should be at end in ControlFileData struct, because for
> crc calculation, it directly take the offset of crc field in
> ControlFileData.
> >
> > /* CRC of all above ... MUST BE LAST! */
> > pg_crc32 crc;
> > +
> > + /* Enable logging WAL when updating hint bits */
> > + bool wal_log_hintbits;
> > } ControlFileData;
> >
> > 3. wal_log_hintbits field should be printed in PrintControlValues
> function.
> >
>
> Thank you for reviewing the patch!
> I have modified the patch base on your comment, and I attached the v7
> patch.

Thanks, patch Looks fine to me, Marked as Ready for Committer.

Regards,
Dilip

On Fri, Dec 13, 2013 at 5:49 PM, Dilip kumar <dilip(dot)kumar(at)huawei(dot)com> wrote:
> On Fri, Dec 13, 2013 at 11:25, Sawada Masahiko Wrote
>
>> >> I attached the patch which have modified based on Robert suggestion,
>> >> and fixed typo.
>> >
>> > I have reviewed the modified patch and I have some comments..
>> >
>> > 1. Patch need to be rebased (failed applying on head)
>> >
>> > 2. crc field should be at end in ControlFileData struct, because for
>> crc calculation, it directly take the offset of crc field in
>> ControlFileData.
>> >
>> > /* CRC of all above ... MUST BE LAST! */
>> > pg_crc32 crc;
>> > +
>> > + /* Enable logging WAL when updating hint bits */
>> > + bool wal_log_hintbits;
>> > } ControlFileData;
>> >
>> > 3. wal_log_hintbits field should be printed in PrintControlValues
>> function.
>> >
>>
>> Thank you for reviewing the patch!
>> I have modified the patch base on your comment, and I attached the v7
>> patch.
>
> Thanks, patch Looks fine to me, Marked as Ready for Committer.
>

I really appreciate your reviewing the patch many times!

Regards,

-------
Sawada Masahiko

On 12/13/2013 07:55 AM, Sawada Masahiko wrote:
> On Fri, Dec 13, 2013 at 1:51 PM, Dilip kumar <dilip(dot)kumar(at)huawei(dot)com> wrote:
>> On 04 December 2013, Sawada Masahiko Wrote
>>
>>> I attached the patch which have modified based on Robert suggestion,
>>> and fixed typo.
>>
>> I have reviewed the modified patch and I have some comments..
>>
>> 1. Patch need to be rebased (failed applying on head)
>>
>> 2. crc field should be at end in ControlFileData struct, because for crc calculation, it directly take the offset of crc field in ControlFileData.
>>
>> /* CRC of all above ... MUST BE LAST! */
>> pg_crc32 crc;
>> +
>> + /* Enable logging WAL when updating hint bits */
>> + bool wal_log_hintbits;
>> } ControlFileData;
>> 3. wal_log_hintbits field should be printed in PrintControlValues function.

Actually, no. It's reset anyway like wal_level and some other settings,
so it's not an interesting value to print out.

Thanks for the review!

> I have modified the patch base on your comment, and I attached the v7 patch.

Thanks, committed with some minor changes:

The amount of extra WAL-logging with wal_log_hintbits=on is almost, but
not exactly the same as with checksums enabled. With checksums enabled,
visibilitymap_set() creates a full-page image of the heap page, but with
wal_log_hintbits=on, it does not. For pg_rewind's purposes, that's
correct, but I feel that it would be better if the decision on whether
to WAL-log or not was exactly the same in both cases. One reason is that
you could then use wal_log_hintbits=on to evaluate how big the impact on
WAL volume would be if you had checksums enabled. I committed it that way.

OTOH, for pg_rewind's purposes, there's actually no need to take a full
page image when a hint bit is set. A small WAL-record indicating that
the page was modified would be enough. It's particularly strange to
insist that hint bit updates create full-page images, when you have
full_page_writes=off so that normal updates don't create them.

We're a bit schizophrenic with full page writes also when data checksums
are enabled, though. If I'm reading the code correctly, you can turn
full_page_writes=off even when data checksums are enabled, which exposes
you to torn page problems. Which might be OK under some special
circumstances. But you'll still get full-page images of hint bits!

I think it's a bit excessive to require wal_level > minimal if you set
wal_log_hintbits=on. It's a bit silly to ask for wal_log_hintbits=on
without archiving, but I also don't see a big reason to throw an error.
It would be annoying, if you want to e.g temporarily set
wal_level=minimal when you do a big data load, and re-initialize the
standby afterwards. Now you'd also need to remember to turn
wal_log_hintbits=off temporarily, and remember to turn it back on
afterwards. So I just removed that check.

I'm not totally satisfied with the name of the GUC, wal_log_hintbits.
The term "hint bits" doesn't mean anything to most people. I couldn't
come up with anything better, but if someone has a better suggestion, we
can still change it.

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> I'm not totally satisfied with the name of the GUC, wal_log_hintbits.

Me either; at the very least, it's short an underscore: wal_log_hint_bits
would be more readable. But how about just "wal_log_hints"?

regards, tom lane

On Fri, Dec 13, 2013 at 10:14:06AM -0500, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> > I'm not totally satisfied with the name of the GUC, wal_log_hintbits.
>
> Me either; at the very least, it's short an underscore: wal_log_hint_bits
> would be more readable. But how about just "wal_log_hints"?

Is wal_log redundant (two "log"s)? How about wal_record_hit_bits?

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

+ Everyone has their own god. +

On Fri, Dec 13, 2013 at 7:57 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> On 12/13/2013 07:55 AM, Sawada Masahiko wrote:
>>
>> On Fri, Dec 13, 2013 at 1:51 PM, Dilip kumar <dilip(dot)kumar(at)huawei(dot)com>
>> wrote:
>>>
>>> On 04 December 2013, Sawada Masahiko Wrote
>>>
>> I have modified the patch base on your comment, and I attached the v7
>> patch.
>
>
> Thanks, committed with some minor changes:

Should this patch in CF app be moved to Committed Patches or is there
something left for this patch?

>> I'm not totally satisfied with the name of the GUC, wal_log_hintbits.

> Me either; at the very least, it's short an underscore: wal_log_hint_bits
> would be more readable. But how about just "wal_log_hints"?

+1 for wal_log_hints, it sounds better.

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

On Wed, Dec 18, 2013 at 11:22 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Dec 13, 2013 at 7:57 PM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
>> Thanks, committed with some minor changes:
>
> Should this patch in CF app be moved to Committed Patches or is there
> something left for this patch?
Nothing has been forgotten for this patch. It can be marked as committed.
--
Michael

On Tue, Dec 17, 2013 at 10:22 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> Me either; at the very least, it's short an underscore: wal_log_hint_bits
>> would be more readable. But how about just "wal_log_hints"?
>
> +1 for wal_log_hints, it sounds better.

+1.

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

2013/12/14 0:14 "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> > I'm not totally satisfied with the name of the GUC, wal_log_hintbits.
>
> Me either; at the very least, it's short an underscore: wal_log_hint_bits
> would be more readable. But how about just "wal_log_hints"?
>

+1 too.
it's readble.

Masahiko Sawada

On Wed, Dec 18, 2013 at 11:30 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Dec 18, 2013 at 11:22 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Fri, Dec 13, 2013 at 7:57 PM, Heikki Linnakangas
>> <hlinnakangas(at)vmware(dot)com> wrote:
>>> Thanks, committed with some minor changes:
>>
>> Should this patch in CF app be moved to Committed Patches or is there
>> something left for this patch?
> Nothing has been forgotten for this patch. It can be marked as committed.

Thanks for confirmation, I have marked it as Committed.

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

On Thu, Dec 19, 2013 at 12:37 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Dec 18, 2013 at 11:30 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Wed, Dec 18, 2013 at 11:22 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> On Fri, Dec 13, 2013 at 7:57 PM, Heikki Linnakangas
>>> <hlinnakangas(at)vmware(dot)com> wrote:
>>>> Thanks, committed with some minor changes:
>>>
>>> Should this patch in CF app be moved to Committed Patches or is there
>>> something left for this patch?
>> Nothing has been forgotten for this patch. It can be marked as committed.
>
> Thanks for confirmation, I have marked it as Committed.
>

Thanks!

I attached the patch which changes name from 'wal_log_hintbits' to
'wal_log_hints'.
It gained the approval of plural.

Regards,

-------
Sawada Masahiko

On Fri, Dec 20, 2013 at 3:38 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Thu, Dec 19, 2013 at 12:37 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Wed, Dec 18, 2013 at 11:30 AM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> On Wed, Dec 18, 2013 at 11:22 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>>> On Fri, Dec 13, 2013 at 7:57 PM, Heikki Linnakangas
>>>> <hlinnakangas(at)vmware(dot)com> wrote:
>>>>> Thanks, committed with some minor changes:
>>>>
>>>> Should this patch in CF app be moved to Committed Patches or is there
>>>> something left for this patch?
>>> Nothing has been forgotten for this patch. It can be marked as committed.
>>
>> Thanks for confirmation, I have marked it as Committed.
>>
>
> Thanks!
>
> I attached the patch which changes name from 'wal_log_hintbits' to
> 'wal_log_hints'.
> It gained the approval of plural.
>

Sorry the patch which I attached has wrong indent on pg_controldata.
I have modified it and attached the new version patch.

Regards,

-------
Sawada Masahiko

On Fri, Dec 20, 2013 at 1:05 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Fri, Dec 20, 2013 at 3:38 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> On Thu, Dec 19, 2013 at 12:37 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> On Wed, Dec 18, 2013 at 11:30 AM, Michael Paquier
>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>> On Wed, Dec 18, 2013 at 11:22 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>>>> On Fri, Dec 13, 2013 at 7:57 PM, Heikki Linnakangas
>>>>> <hlinnakangas(at)vmware(dot)com> wrote:
>>>>>> Thanks, committed with some minor changes:
>>>>>
>>>>> Should this patch in CF app be moved to Committed Patches or is there
>>>>> something left for this patch?
>>>> Nothing has been forgotten for this patch. It can be marked as committed.
>>>
>>> Thanks for confirmation, I have marked it as Committed.
>>>
>>
>> Thanks!
>>
>> I attached the patch which changes name from 'wal_log_hintbits' to
>> 'wal_log_hints'.
>> It gained the approval of plural.
>>
>
> Sorry the patch which I attached has wrong indent on pg_controldata.
> I have modified it and attached the new version patch.
Now that you send this patch, I am just recalling some recent email
from Tom arguing about avoiding to mix lower and upper-case characters
for a GUC parameter name:
http://www.postgresql.org/message-id/30569.1384917859@sss.pgh.pa.us

To fullfill this requirement, could you replace walLogHints by
wal_log_hints in your patch? Thoughts from others?
Regards,
--
Michael

Michael Paquier escribió:
> On Fri, Dec 20, 2013 at 1:05 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:

> > Sorry the patch which I attached has wrong indent on pg_controldata.
> > I have modified it and attached the new version patch.
> Now that you send this patch, I am just recalling some recent email
> from Tom arguing about avoiding to mix lower and upper-case characters
> for a GUC parameter name:
> http://www.postgresql.org/message-id/30569.1384917859@sss.pgh.pa.us
>
> To fullfill this requirement, could you replace walLogHints by
> wal_log_hints in your patch? Thoughts from others?

The issue is with the user-visible variables, not with internal
variables implementing them. I think the patch is sane. (Other than
the fact that it was posted as a patch-on-patch instead of
patch-on-master).

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

On Fri, Dec 20, 2013 at 2:05 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Fri, Dec 20, 2013 at 3:38 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> On Thu, Dec 19, 2013 at 12:37 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> On Wed, Dec 18, 2013 at 11:30 AM, Michael Paquier
>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>> On Wed, Dec 18, 2013 at 11:22 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>>>> On Fri, Dec 13, 2013 at 7:57 PM, Heikki Linnakangas
>>>>> <hlinnakangas(at)vmware(dot)com> wrote:
>>>>>> Thanks, committed with some minor changes:
>>>>>
>>>>> Should this patch in CF app be moved to Committed Patches or is there
>>>>> something left for this patch?
>>>> Nothing has been forgotten for this patch. It can be marked as committed.
>>>
>>> Thanks for confirmation, I have marked it as Committed.
>>>
>>
>> Thanks!
>>
>> I attached the patch which changes name from 'wal_log_hintbits' to
>> 'wal_log_hints'.
>> It gained the approval of plural.
>>
>
> Sorry the patch which I attached has wrong indent on pg_controldata.
> I have modified it and attached the new version patch.

Thanks! Committed.

Regards,

--
Fujii Masao

On Fri, Dec 20, 2013 at 9:06 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Michael Paquier escribió:
>> On Fri, Dec 20, 2013 at 1:05 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
>> > Sorry the patch which I attached has wrong indent on pg_controldata.
>> > I have modified it and attached the new version patch.
>> Now that you send this patch, I am just recalling some recent email
>> from Tom arguing about avoiding to mix lower and upper-case characters
>> for a GUC parameter name:
>> http://www.postgresql.org/message-id/30569.1384917859@sss.pgh.pa.us
>>
>> To fullfill this requirement, could you replace walLogHints by
>> wal_log_hints in your patch? Thoughts from others?
>
> The issue is with the user-visible variables, not with internal
> variables implementing them. I think the patch is sane. (Other than
> the fact that it was posted as a patch-on-patch instead of
> patch-on-master).

But spelling it the same way everywhere really improves greppability.

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

On 12/20/2013 08:34 PM, Fujii Masao wrote:
> On Fri, Dec 20, 2013 at 2:05 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> On Fri, Dec 20, 2013 at 3:38 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> I attached the patch which changes name from 'wal_log_hintbits' to
>>> 'wal_log_hints'.
>>> It gained the approval of plural.
>>
>> Sorry the patch which I attached has wrong indent on pg_controldata.
>> I have modified it and attached the new version patch.
>
> Thanks! Committed.

Thanks. I was hoping someone would come up with an even better name, but
since no-one did, I agree that's better.

- Heikki

On Sat, Dec 21, 2013 at 4:13 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Dec 20, 2013 at 9:06 AM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
>> Michael Paquier escribió:
>>> On Fri, Dec 20, 2013 at 1:05 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>
>>> > Sorry the patch which I attached has wrong indent on pg_controldata.
>>> > I have modified it and attached the new version patch.
>>> Now that you send this patch, I am just recalling some recent email
>>> from Tom arguing about avoiding to mix lower and upper-case characters
>>> for a GUC parameter name:
>>> http://www.postgresql.org/message-id/30569.1384917859@sss.pgh.pa.us
>>>
>>> To fullfill this requirement, could you replace walLogHints by
>>> wal_log_hints in your patch? Thoughts from others?
>>
>> The issue is with the user-visible variables, not with internal
>> variables implementing them. I think the patch is sane. (Other than
>> the fact that it was posted as a patch-on-patch instead of
>> patch-on-master).
>
> But spelling it the same way everywhere really improves greppability.
Yep, finding all the code paths with a single keyword is usually a
good thing. Attached is a purely-aesthetical patch that updates the
internal variable name to wal_log_hints.
--
Michael

On Tue, Dec 24, 2013 at 1:58 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Sat, Dec 21, 2013 at 4:13 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Dec 20, 2013 at 9:06 AM, Alvaro Herrera
>> <alvherre(at)2ndquadrant(dot)com> wrote:
>>> Michael Paquier escribió:
>>>> On Fri, Dec 20, 2013 at 1:05 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>
>>>> > Sorry the patch which I attached has wrong indent on pg_controldata.
>>>> > I have modified it and attached the new version patch.
>>>> Now that you send this patch, I am just recalling some recent email
>>>> from Tom arguing about avoiding to mix lower and upper-case characters
>>>> for a GUC parameter name:
>>>> http://www.postgresql.org/message-id/30569.1384917859@sss.pgh.pa.us
>>>>
>>>> To fullfill this requirement, could you replace walLogHints by
>>>> wal_log_hints in your patch? Thoughts from others?
>>>
>>> The issue is with the user-visible variables, not with internal
>>> variables implementing them. I think the patch is sane. (Other than
>>> the fact that it was posted as a patch-on-patch instead of
>>> patch-on-master).
>>
>> But spelling it the same way everywhere really improves greppability.
> Yep, finding all the code paths with a single keyword is usually a
> good thing. Attached is a purely-aesthetical patch that updates the
> internal variable name to wal_log_hints.

There are many GUC parameters other than wal_log_hints, that their
names are not the same as the names of their variables. We should
update also them?

Regards,

--
Fujii Masao

On Wed, Dec 25, 2013 at 2:36 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, Dec 24, 2013 at 1:58 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Sat, Dec 21, 2013 at 4:13 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Fri, Dec 20, 2013 at 9:06 AM, Alvaro Herrera
>>> <alvherre(at)2ndquadrant(dot)com> wrote:
>>>> Michael Paquier escribió:
>>>>> On Fri, Dec 20, 2013 at 1:05 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>>
>>>>> > Sorry the patch which I attached has wrong indent on pg_controldata.
>>>>> > I have modified it and attached the new version patch.
>>>>> Now that you send this patch, I am just recalling some recent email
>>>>> from Tom arguing about avoiding to mix lower and upper-case characters
>>>>> for a GUC parameter name:
>>>>> http://www.postgresql.org/message-id/30569.1384917859@sss.pgh.pa.us
>>>>>
>>>>> To fullfill this requirement, could you replace walLogHints by
>>>>> wal_log_hints in your patch? Thoughts from others?
>>>>
>>>> The issue is with the user-visible variables, not with internal
>>>> variables implementing them. I think the patch is sane. (Other than
>>>> the fact that it was posted as a patch-on-patch instead of
>>>> patch-on-master).
>>>
>>> But spelling it the same way everywhere really improves greppability.
>> Yep, finding all the code paths with a single keyword is usually a
>> good thing. Attached is a purely-aesthetical patch that updates the
>> internal variable name to wal_log_hints.
>
> There are many GUC parameters other than wal_log_hints, that their
> names are not the same as the names of their variables. We should
> update also them?
IMO, this looks hard to accept as some existing extensions would break
(even if fix would be trivial) and it would make back-patching more
difficult. wal_log_hints is a new parameter though...
Regards,
--
Michael

On Tue, Dec 24, 2013 at 7:31 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
>>> Yep, finding all the code paths with a single keyword is usually a
>>> good thing. Attached is a purely-aesthetical patch that updates the
>>> internal variable name to wal_log_hints.
>>
>> There are many GUC parameters other than wal_log_hints, that their
>> names are not the same as the names of their variables. We should
>> update also them?
> IMO, this looks hard to accept as some existing extensions would break
> (even if fix would be trivial) and it would make back-patching more
> difficult. wal_log_hints is a new parameter though...

That's pretty much how I feel about it as well. It probably wouldn't
hurt very much to go and rename things like Log_disconnections to
log_disconnections, but changing NBuffers to shared_buffers would
doubtless annoy a lot of people. Rather than argue about it, I
suppose we might as well leave the old ones alone.

But keeping the names consistent for new parameters seems simple
enough, so I've committed your patch.

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

On Thu, Jan 2, 2014 at 10:21 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Dec 24, 2013 at 7:31 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>> Yep, finding all the code paths with a single keyword is usually a
>>>> good thing. Attached is a purely-aesthetical patch that updates the
>>>> internal variable name to wal_log_hints.
>>>
>>> There are many GUC parameters other than wal_log_hints, that their
>>> names are not the same as the names of their variables. We should
>>> update also them?
>> IMO, this looks hard to accept as some existing extensions would break
>> (even if fix would be trivial) and it would make back-patching more
>> difficult. wal_log_hints is a new parameter though...
>
> That's pretty much how I feel about it as well. It probably wouldn't
> hurt very much to go and rename things like Log_disconnections to
> log_disconnections, but changing NBuffers to shared_buffers would
> doubtless annoy a lot of people. Rather than argue about it, I
> suppose we might as well leave the old ones alone.
>
> But keeping the names consistent for new parameters seems simple
> enough, so I've committed your patch.
Thanks.
--
Michael