Thread: ALTER SYSTEM SET typos and fix for temporary file name management
Hi all, After going through commit 65d6e4c (introducing ALTER SYSTEM SET), I noticed a couple of typo mistakes as well as (I think) a weird way of using the temporary auto-configuration name postgresql.auto.conf.temp in two different places, resulting in the patch attached. It might be an overkill to use a dedicated variable for the temporary autoconfiguration file (here PG_AUTOCONF_FILENAME_TEMP), but I found kind of weird the way things are currently done on master branch. IMO, it would reduce bug possibilities to have everything managed with a single variable. Typos at least should be fixed :) Regards, -- Michael
Attachment
On Sat, Jan 18, 2014 at 7:59 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > Hi all, > > After going through commit 65d6e4c (introducing ALTER SYSTEM SET), I > noticed a couple of typo mistakes as well as (I think) a weird way of > using the temporary auto-configuration name postgresql.auto.conf.temp > in two different places, resulting in the patch attached. .tmp suffix is used at couple other places in code as well. snapmgr.c XactExportFilePath(pathtmp, topXid, list_length(exportedSnapshots), ".tmp"); receivelog.c snprintf(tmppath, MAXPGPATH, "%s.tmp", path); Although similar suffix is used at other places, but still I think it might be better to define for current case as here prefix (postgresql.auto.conf) is also fixed and chances of getting it changed are less. However even if we don't do, it might be okay as we are using such suffixes at other places as well. > It might be an overkill to use a dedicated variable for the temporary > autoconfiguration file (here PG_AUTOCONF_FILENAME_TEMP), but I found > kind of weird the way things are currently done on master branch. IMO, > it would reduce bug possibilities to have everything managed with a > single variable. > > Typos at least should be fixed :) - appendStringInfoString(&buf, "# Do not edit this file manually! \n"); - appendStringInfoString(&buf, "# It will be overwritten by ALTER SYSTEM command. \n"); + appendStringInfoString(&buf, "# Do not edit this file manually!\n"); + appendStringInfoString(&buf, "# It will be overwritten by ALTER SYSTEM command.\n"); Same change in initdb.c is missing. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Mon, Jan 20, 2014 at 2:12 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Sat, Jan 18, 2014 at 7:59 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> Hi all, >> >> After going through commit 65d6e4c (introducing ALTER SYSTEM SET), I >> noticed a couple of typo mistakes as well as (I think) a weird way of >> using the temporary auto-configuration name postgresql.auto.conf.temp >> in two different places, resulting in the patch attached. > > .tmp suffix is used at couple other places in code as well. > snapmgr.c > XactExportFilePath(pathtmp, topXid, list_length(exportedSnapshots), ".tmp"); > receivelog.c > snprintf(tmppath, MAXPGPATH, "%s.tmp", path); > > Although similar suffix is used at other places, but still I think it might be > better to define for current case as here prefix (postgresql.auto.conf) is also > fixed and chances of getting it changed are less. However even if we don't > do, it might be okay as we are using such suffixes at other places as well. In the case of snapmgr.c and receivelog.c, the operations are kept local, so it does not matter much if this way of naming is done as all the operations for a temporary file are kept within the same, isolated function. However, the case of postgresql.auto.conf.temp is different: this temporary file name is used as well for a check in basebackup.c, so I imagine that it would be better to centralize this information in a single place. Now this name is only used in two places, but who knows if some additional checks here are there won't be needed for this temp file... postgresql.auto.conf.temp is also located at the root of PGDATA, so it might be surprising for the user to find it even if there are few chances that it can happen. >> It might be an overkill to use a dedicated variable for the temporary >> autoconfiguration file (here PG_AUTOCONF_FILENAME_TEMP), but I found >> kind of weird the way things are currently done on master branch. IMO, >> it would reduce bug possibilities to have everything managed with a >> single variable. >> >> Typos at least should be fixed :) > > - appendStringInfoString(&buf, "# Do not edit this file manually! \n"); > - appendStringInfoString(&buf, "# It will be overwritten by ALTER > SYSTEM command. \n"); > + appendStringInfoString(&buf, "# Do not edit this file manually!\n"); > + appendStringInfoString(&buf, "# It will be overwritten by ALTER > SYSTEM command.\n"); > > Same change in initdb.c is missing. Thanks, I missed it. -- Michael
Attachment
On Tue, Jan 21, 2014 at 7:47 AM, Michael Paquier <michael.paquier@gmail.com> wrote: >>> After going through commit 65d6e4c (introducing ALTER SYSTEM SET), I >>> noticed a couple of typo mistakes as well as (I think) a weird way of >>> using the temporary auto-configuration name postgresql.auto.conf.temp >>> in two different places, resulting in the patch attached. >> >> .tmp suffix is used at couple other places in code as well. >> snapmgr.c >> XactExportFilePath(pathtmp, topXid, list_length(exportedSnapshots), ".tmp"); >> receivelog.c >> snprintf(tmppath, MAXPGPATH, "%s.tmp", path); >> >> Although similar suffix is used at other places, but still I think it might be >> better to define for current case as here prefix (postgresql.auto.conf) is also >> fixed and chances of getting it changed are less. However even if we don't >> do, it might be okay as we are using such suffixes at other places as well. > In the case of snapmgr.c and receivelog.c, the operations are kept > local, so it does not matter much if this way of naming is done as all > the operations for a temporary file are kept within the same, isolated > function. However, the case of postgresql.auto.conf.temp is different: > this temporary file name is used as well for a check in basebackup.c, > so I imagine that it would be better to centralize this information in > a single place. Now this name is only used in two places, but who > knows if some additional checks here are there won't be needed for > this temp file... > postgresql.auto.conf.temp is also located at the root of PGDATA, so it > might be surprising for the user to find it even if there are few > chances that it can happen. I don't think there's any real reason to defined PG_AUTOCONF_FILENAME_TEMP. pg_stat_statements just writes PGSS_DUMP_FILE ".tmp" and that hasn't been a problem that I know of. I do wonder why ALTER SYSTEM SET is spelling the suffix "temp" instead of "tmp". -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas escribió: > I don't think there's any real reason to defined > PG_AUTOCONF_FILENAME_TEMP. pg_stat_statements just writes > PGSS_DUMP_FILE ".tmp" and that hasn't been a problem that I know of. > I do wonder why ALTER SYSTEM SET is spelling the suffix "temp" instead > of "tmp". I agree with Michael that having pg_basebackup be aware of the ".temp" suffix is ugly; for instance if we were to fix it to ".tmp" in ALTER SYSTEM but forgot to change pg_basebackup, the check would be immediately broken. But on the other hand I'm not sure why it's such a problem for pg_basebackup that it needs to be checking in the first place -- how about we rip that out? Perhaps the temp file needs to be in pgsql_tmp? (Do we need to worry about cross-filesystem links if we do? I mean, do we support mounting pgsql_tmp separately?) -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Jan 22, 2014 at 5:29 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > I agree with Michael that having pg_basebackup be aware of the ".temp" > suffix is ugly; for instance if we were to fix it to ".tmp" in ALTER > SYSTEM but forgot to change pg_basebackup, the check would be > immediately broken. But on the other hand I'm not sure why it's such a > problem for pg_basebackup that it needs to be checking in the first > place -- how about we rip that out? Coupled with the addition of a pgsql_tmp prefix to the temp configuration file name would work, yes we could remove this check part. > Perhaps the temp file needs to be in pgsql_tmp? (Do we need to worry > about cross-filesystem links if we do? I mean, do we support mounting > pgsql_tmp separately?) Using pgsql_tmp looks a bit weird as well, as this prefix is used only for temporary files doing database-related operations, and ALTER SYSTEM is not one. Doing that this would need a mention in the docs at least: http://www.postgresql.org/docs/devel/static/storage-file-layout.html Thoughts? Regards, -- Michael
On Wed, Jan 22, 2014 at 1:15 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Jan 21, 2014 at 7:47 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >>>> After going through commit 65d6e4c (introducing ALTER SYSTEM SET), I >>>> noticed a couple of typo mistakes as well as (I think) a weird way of >>>> using the temporary auto-configuration name postgresql.auto.conf.temp >>>> in two different places, resulting in the patch attached. >>> >>> .tmp suffix is used at couple other places in code as well. >>> snapmgr.c >>> XactExportFilePath(pathtmp, topXid, list_length(exportedSnapshots), ".tmp"); >>> receivelog.c >>> snprintf(tmppath, MAXPGPATH, "%s.tmp", path); >>> >>> Although similar suffix is used at other places, but still I think it might be >>> better to define for current case as here prefix (postgresql.auto.conf) is also >>> fixed and chances of getting it changed are less. However even if we don't >>> do, it might be okay as we are using such suffixes at other places as well. >> In the case of snapmgr.c and receivelog.c, the operations are kept >> local, so it does not matter much if this way of naming is done as all >> the operations for a temporary file are kept within the same, isolated >> function. However, the case of postgresql.auto.conf.temp is different: >> this temporary file name is used as well for a check in basebackup.c, >> so I imagine that it would be better to centralize this information in >> a single place. Now this name is only used in two places, but who >> knows if some additional checks here are there won't be needed for >> this temp file... >> postgresql.auto.conf.temp is also located at the root of PGDATA, so it >> might be surprising for the user to find it even if there are few >> chances that it can happen. > > I don't think there's any real reason to defined > PG_AUTOCONF_FILENAME_TEMP. pg_stat_statements just writes > PGSS_DUMP_FILE ".tmp" and that hasn't been a problem that I know of. > I do wonder why ALTER SYSTEM SET is spelling the suffix "temp" instead > of "tmp". +1 for changing '.temp' to '.tmp'. I think its bit arguable whether to change it to define or not (different people can have different opinion on this matter), but certainly changing it to .tmp is an improvement, as we use .tmp at other places as well. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Jan 21, 2014 at 7:02 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Wed, Jan 22, 2014 at 5:29 AM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: >> I agree with Michael that having pg_basebackup be aware of the ".temp" >> suffix is ugly; for instance if we were to fix it to ".tmp" in ALTER >> SYSTEM but forgot to change pg_basebackup, the check would be >> immediately broken. But on the other hand I'm not sure why it's such a >> problem for pg_basebackup that it needs to be checking in the first >> place -- how about we rip that out? > Coupled with the addition of a pgsql_tmp prefix to the temp > configuration file name would work, yes we could remove this check > part. > >> Perhaps the temp file needs to be in pgsql_tmp? (Do we need to worry >> about cross-filesystem links if we do? I mean, do we support mounting >> pgsql_tmp separately?) > Using pgsql_tmp looks a bit weird as well, as this prefix is used only > for temporary files doing database-related operations, and ALTER > SYSTEM is not one. Doing that this would need a mention in the docs at > least: > http://www.postgresql.org/docs/devel/static/storage-file-layout.html > Thoughts? I think moving the temp file to a different directory than the permanent file is a non-starter. As Alvaro says, that could be on a different file system, and then attempting to rename() the file into place would fail. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, Please find attached an updated patch (context diffs) improving the comments related to ALTER SYSTEM. This patch does nothing for the suffix tmp/temp used in a couple of places of the code, it only corrects some typos and makes the comments more consistent with current code. The inconsistencies with the temporary file suffixes should be tackled in a separate patch/thread. Thanks, -- Michael
Attachment
On Mon, Jan 27, 2014 at 11:29 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > Hi, > > Please find attached an updated patch (context diffs) improving the > comments related to ALTER SYSTEM. This patch does nothing for the > suffix tmp/temp used in a couple of places of the code, it only > corrects some typos and makes the comments more consistent with > current code. And I almost forgot the second patch simply changing the suffix from temp to tmp for the temporary conf file... Now attached. Regards, -- Michael
Attachment
On Mon, Jan 27, 2014 at 11:53 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Mon, Jan 27, 2014 at 11:29 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> Hi, >> >> Please find attached an updated patch (context diffs) improving the >> comments related to ALTER SYSTEM. This patch does nothing for the >> suffix tmp/temp used in a couple of places of the code, it only >> corrects some typos and makes the comments more consistent with >> current code. > And I almost forgot the second patch simply changing the suffix from > temp to tmp for the temporary conf file... Now attached. > Regards, Thanks for the patches! Committed. Regards, -- Fujii Masao
On Mon, Jan 27, 2014 at 12:49 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Mon, Jan 27, 2014 at 11:53 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Mon, Jan 27, 2014 at 11:29 AM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >>> Hi, >>> >>> Please find attached an updated patch (context diffs) improving the >>> comments related to ALTER SYSTEM. This patch does nothing for the >>> suffix tmp/temp used in a couple of places of the code, it only >>> corrects some typos and makes the comments more consistent with >>> current code. >> And I almost forgot the second patch simply changing the suffix from >> temp to tmp for the temporary conf file... Now attached. >> Regards, > > Thanks for the patches! Committed. Thanks. -- Michael