Обсуждение: Typo in description of replay_lag attribute in pg_stat_replicationview
Hello! I have noticed that in description of *flush_lag* in pg_stat_replication view (https://www.postgresql.org/docs/current/monitoring-stats.html#PG-STAT-REPLICATION-VIEW) there exists unknown value of synchronous_commit parameter - *remote_flush*. I think it was meant to use the value *on*. Small patch is attached. -- Regards, Maksim Milyutin
Вложения
On Mon, Dec 03, 2018 at 06:18:30PM +0300, Maksim Milyutin wrote: > I have noticed that in description of *flush_lag* in pg_stat_replication > view (https://www.postgresql.org/docs/current/monitoring-stats.html#PG-STAT-REPLICATION-VIEW) > there exists unknown value of synchronous_commit parameter - *remote_flush*. > I think it was meant to use the value *on*. Yes, you are right. It should be "on" as "remote_flush" is not a valid value. remote_flush is listed in SyncCommitLevel though, so this makes me wonder if we should also introduce a new value for this purpose available for users. The fix you propose looks good to me. Any opinions from others? -- Michael
Вложения
On Tue, Dec 4, 2018 at 1:18 PM Michael Paquier <michael@paquier.xyz> wrote: > On Mon, Dec 03, 2018 at 06:18:30PM +0300, Maksim Milyutin wrote: > > I have noticed that in description of *flush_lag* in pg_stat_replication > > view (https://www.postgresql.org/docs/current/monitoring-stats.html#PG-STAT-REPLICATION-VIEW) > > there exists unknown value of synchronous_commit parameter - *remote_flush*. > > I think it was meant to use the value *on*. > > Yes, you are right. It should be "on" as "remote_flush" is not a valid > value. remote_flush is listed in SyncCommitLevel though, so this makes > me wonder if we should also introduce a new value for this purpose > available for users. The fix you propose looks good to me. Any > opinions from others? +1 for the patch. As for introducing remote_flush as the true name of the level, this was discussed but somehow went off-course and never made it to the finish line: https://www.postgresql.org/message-id/flat/CAEepm%3D3FFaanSS4sugG%2BApzq2tCVjEYCO2wOQBod2d7GWb%3DDvA%40mail.gmail.com -- Thomas Munro http://www.enterprisedb.com
On Tue, Dec 04, 2018 at 01:28:15PM +1300, Thomas Munro wrote: > On Tue, Dec 4, 2018 at 1:18 PM Michael Paquier <michael@paquier.xyz> wrote: >> Yes, you are right. It should be "on" as "remote_flush" is not a valid >> value. remote_flush is listed in SyncCommitLevel though, so this makes >> me wonder if we should also introduce a new value for this purpose >> available for users. The fix you propose looks good to me. Any >> opinions from others? > > +1 for the patch. Thanks for confirming, Thomas. I'll go apply hopefully tomorrow if nobody has objections. > As for introducing remote_flush as the true name of the level, this > was discussed but somehow went off-course and never made it to the > finish line: > > https://www.postgresql.org/message-id/flat/CAEepm%3D3FFaanSS4sugG%2BApzq2tCVjEYCO2wOQBod2d7GWb%3DDvA%40mail.gmail.com Oh, I forgot this one. We may want to revive that... remote_flush is more meaningful than on, especially since there are more and more possible values for synchronous_commit. -- Michael
Вложения
04.12.2018 5:19, Michael Paquier wrote: > On Tue, Dec 04, 2018 at 01:28:15PM +1300, Thomas Munro wrote: >> On Tue, Dec 4, 2018 at 1:18 PM Michael Paquier <michael@paquier.xyz> wrote: >>> Yes, you are right. It should be "on" as "remote_flush" is not a valid >>> value. remote_flush is listed in SyncCommitLevel though, so this makes >>> me wonder if we should also introduce a new value for this purpose >>> available for users. The fix you propose looks good to me. Any >>> opinions from others? >> +1 for the patch. > Thanks for confirming, Thomas. I'll go apply hopefully tomorrow if > nobody has objections. > >> As for introducing remote_flush as the true name of the level, this >> was discussed but somehow went off-course and never made it to the >> finish line: >> >> https://www.postgresql.org/message-id/flat/CAEepm%3D3FFaanSS4sugG%2BApzq2tCVjEYCO2wOQBod2d7GWb%3DDvA%40mail.gmail.com > Oh, I forgot this one. We may want to revive that... remote_flush is > more meaningful than on, especially since there are more and more > possible values for synchronous_commit. Yeah, I think the notion *remote_flush level* is more appropriate especially in the context of sync replication. Within this context maybe it makes sense to replace the word *level* to *value* in description of *flush_lag*? -- Regards, Maksim Milyutin
On Wed, Dec 05, 2018 at 01:24:22AM +0300, Maksim Milyutin wrote: > Yeah, I think the notion *remote_flush level* is more appropriate especially > in the context of sync replication. Within this context maybe it makes sense > to replace the word *level* to *value* in description of *flush_lag*? I am not sure that this is an improvement. Anyway, I have committed your original patch as that's clearly a mistake and back-patched down to v10. -- Michael
Вложения
05.12.2018 4:04, Michael Paquier wrote: > On Wed, Dec 05, 2018 at 01:24:22AM +0300, Maksim Milyutin wrote: >> Yeah, I think the notion *remote_flush level* is more appropriate especially >> in the context of sync replication. Within this context maybe it makes sense >> to replace the word *level* to *value* in description of *flush_lag*? > I am not sure that this is an improvement. Anyway, I have committed > your original patch as that's clearly a mistake and back-patched down to > v10. Ok, thanks. -- Regards, Maksim Milyutin