Обсуждение: TODO: Split out pg_resetxlog output into pre- and post-sections
This patch implements the following TODO item:
Split out pg_resetxlog output into pre- and post-sections
http://archives.postgresql.org/pgsql-hackers/2010-08/msg02040.php
On execution of pg_resetxlog using the option -n
1. It will display values in two section.
2. First section will be called as "Current pg_control values or Guess pg_control values".
3. In first section, it will display all current (i.e. before change) values of control file or guessed values.
4. Second section will be called as "Values to be used after reset".
5. In second section, it will display new values of parameters to be reset as per user request.
Please provide your opinion or expectation out of this patch.
I will add the same to November commitFest.
Thanks and Regards,
Kumar Rajeev Rastogi
Вложения
On Tue, Nov 5, 2013 at 3:20 PM, Rajeev rastogi <rajeev.rastogi@huawei.com> wrote: > On execution of pg_resetxlog using the option -n > > 1. It will display values in two section. > > 2. First section will be called as "Current pg_control > values or Guess pg_control values". > > 3. In first section, it will display all current (i.e. > before change) values of control file or guessed values. > > 4. Second section will be called as "Values to be used after > reset". > > 5. In second section, it will display new values of > parameters to be reset as per user request. > > > > Please provide your opinion or expectation out of this patch. Your approach in patch seems to be inline with Todo item. On a quick glance, I observed few things which can make your patch better: 1. The purpose was to print pg_control values in one section and any other reset values in different section, so in that regard, should we display below in new section, as here newXlogSegNo is not directly from pg_control. PrintControlValues() { .. XLogFileName(fname, ControlFile.checkPointCopy.ThisTimeLineID, newXlogSegNo); printf(_("First log segment after reset: %s\n"), fname); } 2. why to have extra flags for changedParam, can't we do without it. For example, we already use set_xid value to check if user has provided xid. 3. + static void + PrintNewControlValues(int changedParam) + { + if (changedParam) + printf(_("\n\nValues to be used after reset:\n\n")); Even after first if check fails, still you continue to check other values, it is better if you can have check if (changedParam) before calling this function With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Fri, 08 November 2013 09:47 > On Tue, Nov 5, 2013 at 3:20 PM, Rajeev rastogi > <rajeev.rastogi@huawei.com> wrote: > > On execution of pg_resetxlog using the option -n > > > > 1. It will display values in two section. > > > > 2. First section will be called as "Current > pg_control > > values or Guess pg_control values". > > > > 3. In first section, it will display all current (i.e. > > before change) values of control file or guessed values. > > > > 4. Second section will be called as "Values to be > used > > after reset". > > > > 5. In second section, it will display new values of > > parameters to be reset as per user request. > > > > > > > > Please provide your opinion or expectation out of this patch. > > Your approach in patch seems to be inline with Todo item. On a quick > glance, I observed few things which can make your patch better: > > 1. The purpose was to print pg_control values in one section and any > other reset values in different section, so in that > regard, should we display below in new section, as here newXlogSegNo > is not directly from pg_control. > > PrintControlValues() > { > .. > XLogFileName(fname, ControlFile.checkPointCopy.ThisTimeLineID, > newXlogSegNo); > > printf(_("First log segment after reset: %s\n"), > fname); > } Yes we can print newXlogSegNo. > 2. why to have extra flags for changedParam, can't we do without it. > For example, we already use set_xid value to check if user has provided > xid. You mean to say that we can print wherever values of control file parameter is getting assigned. If yes, then the problem is that every places will have to check the condition as if(noupdate) and then print the corresponding value. E.g. If (noupdate) printf(_("NextMultiXactId:%u\n"), ControlFile.checkPointCopy.nextMulti); > 3. > + static void > + PrintNewControlValues(int changedParam) { > + if (changedParam) > + printf(_("\n\nValues to be used after reset:\n\n")); > > Even after first if check fails, still you continue to check other > values, it is better if you can have check if (changedParam) before > calling this function Yes you are correct. But now this check will not be required because always at-least one value (i.e. of newXlogSegNo) will be printed. Please let me know if understanding of all comments are correct. After that I will update the patch. Thanks and Regards, Kumar Rajeev Rastogi
On Fri, Nov 8, 2013 at 10:37 AM, Rajeev rastogi <rajeev.rastogi@huawei.com> wrote: > On Fri, 08 November 2013 09:47 > >> On Tue, Nov 5, 2013 at 3:20 PM, Rajeev rastogi >> <rajeev.rastogi@huawei.com> wrote: >> > On execution of pg_resetxlog using the option -n >> > >> > Please provide your opinion or expectation out of this patch. >> >> Your approach in patch seems to be inline with Todo item. On a quick >> glance, I observed few things which can make your patch better: >> >> 1. The purpose was to print pg_control values in one section and any >> other reset values in different section, so in that >> regard, should we display below in new section, as here newXlogSegNo >> is not directly from pg_control. >> >> PrintControlValues() >> { >> .. >> XLogFileName(fname, ControlFile.checkPointCopy.ThisTimeLineID, >> newXlogSegNo); >> >> printf(_("First log segment after reset: %s\n"), >> fname); >> } > > Yes we can print newXlogSegNo. I think then your documentation also need updates. >> 2. why to have extra flags for changedParam, can't we do without it. >> For example, we already use set_xid value to check if user has provided >> xid. > > You mean to say that we can print wherever values of control file parameter is > getting assigned. > > If yes, then the problem is that every places will have to check the condition as > if(noupdate) and then print the corresponding value. E.g. > If (noupdate) > printf(_("NextMultiXactId:%u\n"), ControlFile.checkPointCopy.nextMulti); No, not this way, maybe making set_xid as file level parameters, so that you can use them later as well. Your current way also doesn't seem to be too unreasonable, however it is better if you can do without it. One more thing, I think as per this patch few parameters will be displayed twice once in "pg_control values .." section and once in "Values to be used after reset:", so by doing this I guess you want to make it easier for user to refer both pg_control's original/guessed value and new value after reset. Here I wonder if someone wants to refer to original values, can't he directly use pg_controldata? Anyone else have thoughts about how can we display values which can make current situation better for user. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com