Thread: 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
Attachment
On Thu, Nov 14, 2013 at 3:02 PM, Sawada Masahiko <sawada.mshk@gmail.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.paquier@gmail.com> wrote: > On Thu, Nov 14, 2013 at 3:02 PM, Sawada Masahiko <sawada.mshk@gmail.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@redhat.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@gmx.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
Attachment
(2013/11/15 19:27), Sawada Masahiko wrote: > On Thu, Nov 14, 2013 at 7:51 PM, Florian Weimer <fweimer@redhat.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.mshk@gmail.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.mitsumasa@lab.ntt.co.jp> wrote: > (2013/11/15 19:27), Sawada Masahiko wrote: >> >> On Thu, Nov 14, 2013 at 7:51 PM, Florian Weimer <fweimer@redhat.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 Wed, Nov 20, 2013 at 9:19 PM, Dilip kumar <dilip.kumar@huawei.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 Thu, Nov 21, 2013 at 8:59 PM, Dilip kumar <dilip.kumar@huawei.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 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 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 Tue, 2013-11-19 at 11:42 -0500, Robert Haas wrote: > On Thu, Nov 14, 2013 at 1:02 AM, Sawada Masahiko <sawada.mshk@gmail.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@j-davis.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
Attachment
On Mon, Nov 25, 2013 at 9:02 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Sun, Nov 24, 2013 at 6:02 AM, Jeff Davis <pgsql@j-davis.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@gmx.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@sss.pgh.pa.us> wrote: > Peter Eisentraut <peter_e@gmx.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
Attachment
On Wed, Nov 27, 2013 at 11:04 AM, Sawada Masahiko <sawada.mshk@gmail.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.paquier@gmail.com> wrote: > On Wed, Nov 27, 2013 at 11:04 AM, Sawada Masahiko <sawada.mshk@gmail.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
Attachment
On Thu, Nov 28, 2013 at 9:42 PM, Sawada Masahiko <sawada.mshk@gmail.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.paquier@gmail.com> wrote: > On Thu, Nov 28, 2013 at 9:42 PM, Sawada Masahiko <sawada.mshk@gmail.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@gmail.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.paquier@gmail.com> wrote: > On Tue, Dec 3, 2013 at 1:08 AM, Robert Haas <robertmhaas@gmail.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.paquier@gmail.com> wrote: > On Tue, Dec 3, 2013 at 11:57 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Tue, Dec 3, 2013 at 1:08 AM, Robert Haas <robertmhaas@gmail.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.mshk@gmail.com> wrote: > On Tue, Dec 3, 2013 at 12:02 PM, Michael Paquier > <michael.paquier@gmail.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.paquier@gmail.com> wrote: > On Tue, Dec 3, 2013 at 3:30 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> On Tue, Dec 3, 2013 at 12:02 PM, Michael Paquier >> <michael.paquier@gmail.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.mshk@gmail.com> wrote: > On Tue, Dec 3, 2013 at 4:28 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Tue, Dec 3, 2013 at 3:30 PM, Sawada Masahiko <sawada.mshk@gmail.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
Attachment
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
crcfield 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.kumar@huawei.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 crcfield 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
Attachment
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.kumar@huawei.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.kumar@huawei.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 crcfield 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@vmware.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@vmware.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@momjian.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@vmware.com> wrote: > On 12/13/2013 07:55 AM, Sawada Masahiko wrote: >> >> On Fri, Dec 13, 2013 at 1:51 PM, Dilip kumar <dilip.kumar@huawei.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.kapila16@gmail.com> wrote: > On Fri, Dec 13, 2013 at 7:57 PM, Heikki Linnakangas > <hlinnakangas@vmware.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.kapila16@gmail.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
<p dir="ltr"><br /> 2013/12/14 0:14 "Tom Lane" <<a href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>>:<br /> ><br/> > Heikki Linnakangas <<a href="mailto:hlinnakangas@vmware.com">hlinnakangas@vmware.com</a>> writes:<br/> > > I'm not totally satisfied with the name of the GUC, wal_log_hintbits.<br /> ><br /> > Me either;at the very least, it's short an underscore: wal_log_hint_bits<br /> > would be more readable. But how about just"wal_log_hints"?<br /> ><p dir="ltr">+1 too.<br /> it's readble.<p dir="ltr">Masahiko Sawada
On Wed, Dec 18, 2013 at 11:30 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Wed, Dec 18, 2013 at 11:22 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> On Fri, Dec 13, 2013 at 7:57 PM, Heikki Linnakangas >> <hlinnakangas@vmware.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.kapila16@gmail.com> wrote: > On Wed, Dec 18, 2013 at 11:30 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Wed, Dec 18, 2013 at 11:22 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>> On Fri, Dec 13, 2013 at 7:57 PM, Heikki Linnakangas >>> <hlinnakangas@vmware.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
Attachment
On Fri, Dec 20, 2013 at 3:38 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Thu, Dec 19, 2013 at 12:37 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> On Wed, Dec 18, 2013 at 11:30 AM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >>> On Wed, Dec 18, 2013 at 11:22 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>>> On Fri, Dec 13, 2013 at 7:57 PM, Heikki Linnakangas >>>> <hlinnakangas@vmware.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
Attachment
On Fri, Dec 20, 2013 at 1:05 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Fri, Dec 20, 2013 at 3:38 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> On Thu, Dec 19, 2013 at 12:37 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>> On Wed, Dec 18, 2013 at 11:30 AM, Michael Paquier >>> <michael.paquier@gmail.com> wrote: >>>> On Wed, Dec 18, 2013 at 11:22 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>>>> On Fri, Dec 13, 2013 at 7:57 PM, Heikki Linnakangas >>>>> <hlinnakangas@vmware.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.mshk@gmail.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.mshk@gmail.com> wrote: > On Fri, Dec 20, 2013 at 3:38 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> On Thu, Dec 19, 2013 at 12:37 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>> On Wed, Dec 18, 2013 at 11:30 AM, Michael Paquier >>> <michael.paquier@gmail.com> wrote: >>>> On Wed, Dec 18, 2013 at 11:22 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>>>> On Fri, Dec 13, 2013 at 7:57 PM, Heikki Linnakangas >>>>> <hlinnakangas@vmware.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@2ndquadrant.com> wrote: > Michael Paquier escribió: >> On Fri, Dec 20, 2013 at 1:05 PM, Sawada Masahiko <sawada.mshk@gmail.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.mshk@gmail.com> wrote: >> On Fri, Dec 20, 2013 at 3:38 AM, Sawada Masahiko <sawada.mshk@gmail.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@gmail.com> wrote: > On Fri, Dec 20, 2013 at 9:06 AM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: >> Michael Paquier escribió: >>> On Fri, Dec 20, 2013 at 1:05 PM, Sawada Masahiko <sawada.mshk@gmail.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
Attachment
On Tue, Dec 24, 2013 at 1:58 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Sat, Dec 21, 2013 at 4:13 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Fri, Dec 20, 2013 at 9:06 AM, Alvaro Herrera >> <alvherre@2ndquadrant.com> wrote: >>> Michael Paquier escribió: >>>> On Fri, Dec 20, 2013 at 1:05 PM, Sawada Masahiko <sawada.mshk@gmail.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.fujii@gmail.com> wrote: > On Tue, Dec 24, 2013 at 1:58 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Sat, Dec 21, 2013 at 4:13 AM, Robert Haas <robertmhaas@gmail.com> wrote: >>> On Fri, Dec 20, 2013 at 9:06 AM, Alvaro Herrera >>> <alvherre@2ndquadrant.com> wrote: >>>> Michael Paquier escribió: >>>>> On Fri, Dec 20, 2013 at 1:05 PM, Sawada Masahiko <sawada.mshk@gmail.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.paquier@gmail.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@gmail.com> wrote: > On Tue, Dec 24, 2013 at 7:31 PM, Michael Paquier > <michael.paquier@gmail.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