Re: [PATCHES] Fix pg_ctl restart bug
Bruce Momjian wrote: Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: However, as of 2004-10-15, this has not worked. :-( The attached patch is the one that caused the bug --- on non-Unix systems, SYSTEMQUOTE is , meaning zero-length string. I should have seen the bug when I applied the contributed patch in 2004. So, shouldn't this fix be back-patched? Well, no one has actually complained about the breakage, and it has been a few years. Also there is always the risk of a new bug being introduced, so I am unsure. Why do we need someone to complain? We know the bug is there. Has the code changed a lot in that area? -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix pg_ctl restart bug
Alvaro Herrera wrote: Bruce Momjian wrote: Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: However, as of 2004-10-15, this has not worked. :-( The attached patch is the one that caused the bug --- on non-Unix systems, SYSTEMQUOTE is , meaning zero-length string. I should have seen the bug when I applied the contributed patch in 2004. So, shouldn't this fix be back-patched? Well, no one has actually complained about the breakage, and it has been a few years. Also there is always the risk of a new bug being introduced, so I am unsure. Why do we need someone to complain? We know the bug is there. Has the code changed a lot in that area? Do we have the policy of backpatching every fix? I thought it was only the major bugs we fixed in back branches. If someone wants to backpatch it, feel free to do so. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix pg_ctl restart bug
Bruce Momjian wrote:
, meaning zero-length string. I should have seen the bug when I
applied the contributed patch in 2004.
So, shouldn't this fix be back-patched?
Well, no one has actually complained about the breakage, and it has been
a few years. Also there is always the risk of a new bug being
introduced, so I am unsure.
Why do we need someone to complain? We know the bug is there. Has the
code changed a lot in that area?
Do we have the policy of backpatching every fix? I thought it was only
the major bugs we fixed in back branches. If someone wants to backpatch
it, feel free to do so.
OK, I started looking at what it would take to backpatch this and found
another bug I have fixed in CVS HEAD. What back branchs (8.0-8.3.X) are
doing is pretty odd. On non-Win32 systems, it is looking for the null
byte, then putting a null byte before it, and passing a NULL back as the
options and binary location. The test:
if (postgres_path != NULL)
postgres_path = optline;
is backwards, which means that if in 8.3.X you start the server with any
arguments, like:
/usr/var/local/postgres/bin/postgres -i -o -d5
and you use pg_ctl to specify the binary location:
pg_ctl -p /u/pg/bin/postmaster restart
the server actually fails to restart because it chops off the last byte
(a bug) and the test above is wrong (another bug), and it thinks the
binary name is the full string, in quotes:
/usr/var/local/postgres/bin/postgres -i -o -d
and you get this error from pg_ctl:
sh: /usr/var/local/postgres/bin/postgres -i -o -d: not found
This is more than just ignoring the documentation, it is a failure.
I am attaching a minimal patch that will fix the bug in back branches.
Keep in mind that a patched pg_ctl will not be able to restart a backend
that was not patched.
--
Bruce Momjian [EMAIL PROTECTED]http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Index: src/backend/postmaster/postmaster.c
===
RCS file: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.551
diff -c -c -r1.551 postmaster.c
*** src/backend/postmaster/postmaster.c 11 Jan 2008 00:54:09 - 1.551
--- src/backend/postmaster/postmaster.c 26 Jun 2008 18:53:42 -
***
*** 4163,4169
fprintf(fp, %s, fullprogname);
for (i = 1; i argc; i++)
! fprintf(fp, %s%s%s, SYSTEMQUOTE, argv[i], SYSTEMQUOTE);
fputs(\n, fp);
if (fclose(fp))
--- 4163,4169
fprintf(fp, %s, fullprogname);
for (i = 1; i argc; i++)
! fprintf(fp, \%s\, argv[i]);
fputs(\n, fp);
if (fclose(fp))
Index: src/bin/pg_ctl/pg_ctl.c
===
RCS file: /cvsroot/pgsql/src/bin/pg_ctl/pg_ctl.c,v
retrieving revision 1.92.2.3
diff -c -c -r1.92.2.3 pg_ctl.c
*** src/bin/pg_ctl/pg_ctl.c 29 Feb 2008 23:31:42 - 1.92.2.3
--- src/bin/pg_ctl/pg_ctl.c 26 Jun 2008 18:53:43 -
***
*** 613,627
{
char *arg1;
! arg1 = strchr(optline, *SYSTEMQUOTE);
! if (arg1 == NULL || arg1 == optline)
! post_opts = ;
! else
{
! *(arg1 - 1) = '\0'; /* this should be a
space */
! post_opts = arg1;
}
! if (postgres_path != NULL)
postgres_path = optline;
}
else
--- 613,628
{
char *arg1;
! /*
!* Are we at the first option, as defined by
space and
!* double-quote?
!*/
! if ((arg1 = strstr(optline, \)) != NULL)
{
! *arg1 = '\0'; /* terminate so we get
only program name */
! post_opts = arg1 + 1; /* point past
whitespace */
}
! if (postgres_path == NULL)
postgres_path = optline;
}
else
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix pg_ctl restart bug
Bruce Momjian wrote:
I am attaching a minimal patch that will fix the bug in back branches.
Keep in mind that a patched pg_ctl will not be able to restart a backend
that was not patched.
I think this patch will work for unpatched backends as well. I am still
uncertain if it should be backpatched.
--
Bruce Momjian [EMAIL PROTECTED]http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Index: src/backend/postmaster/postmaster.c
===
RCS file: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.551
diff -c -c -r1.551 postmaster.c
*** src/backend/postmaster/postmaster.c 11 Jan 2008 00:54:09 - 1.551
--- src/backend/postmaster/postmaster.c 26 Jun 2008 19:11:37 -
***
*** 4163,4169
fprintf(fp, %s, fullprogname);
for (i = 1; i argc; i++)
! fprintf(fp, %s%s%s, SYSTEMQUOTE, argv[i], SYSTEMQUOTE);
fputs(\n, fp);
if (fclose(fp))
--- 4163,4169
fprintf(fp, %s, fullprogname);
for (i = 1; i argc; i++)
! fprintf(fp, \%s\, argv[i]);
fputs(\n, fp);
if (fclose(fp))
Index: src/bin/pg_ctl/pg_ctl.c
===
RCS file: /cvsroot/pgsql/src/bin/pg_ctl/pg_ctl.c,v
retrieving revision 1.92.2.3
diff -c -c -r1.92.2.3 pg_ctl.c
*** src/bin/pg_ctl/pg_ctl.c 29 Feb 2008 23:31:42 - 1.92.2.3
--- src/bin/pg_ctl/pg_ctl.c 26 Jun 2008 19:11:37 -
***
*** 613,627
{
char *arg1;
! arg1 = strchr(optline, *SYSTEMQUOTE);
! if (arg1 == NULL || arg1 == optline)
! post_opts = ;
! else
{
! *(arg1 - 1) = '\0'; /* this should be a space */
! post_opts = arg1;
}
! if (postgres_path != NULL)
postgres_path = optline;
}
else
--- 613,629
{
char *arg1;
! /*
! * Are we at the first option, as defined by space and
! * double-quote?
! */
! if ((arg1 = strstr(optline, \)) != NULL ||
! (arg1 = strstr(optline, -)) != NULL)
{
! *arg1 = '\0'; /* terminate so we get only program name */
! post_opts = arg1 + 1; /* point past whitespace */
}
! if (postgres_path == NULL)
postgres_path = optline;
}
else
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix pg_ctl restart bug
Bruce Momjian wrote: Alvaro Herrera wrote: Why do we need someone to complain? We know the bug is there. Has the code changed a lot in that area? Do we have the policy of backpatching every fix? I thought it was only the major bugs we fixed in back branches. If someone wants to backpatch it, feel free to do so. I think the policy is we fix the bugs in supported releases. If you start making exceptions, it becomes needlessly complex. I've always assumed that I'm supposed to backpatch the bugs I fix in HEAD, however far is reasonable. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix pg_ctl restart bug
Alvaro Herrera wrote: Bruce Momjian wrote: Alvaro Herrera wrote: Why do we need someone to complain? We know the bug is there. Has the code changed a lot in that area? Do we have the policy of backpatching every fix? I thought it was only the major bugs we fixed in back branches. If someone wants to backpatch it, feel free to do so. I think the policy is we fix the bugs in supported releases. If you start making exceptions, it becomes needlessly complex. I've always assumed that I'm supposed to backpatch the bugs I fix in HEAD, however far is reasonable. I thought we only backatched major bugs to prevent possible instability when fixing minor bugs. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix pg_ctl restart bug
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: Alvaro Herrera wrote: I've always assumed that I'm supposed to backpatch the bugs I fix in HEAD, however far is reasonable. I thought we only backatched major bugs to prevent possible instability when fixing minor bugs. Actually, Bruce, this *is* a minor bug; if it were major we'd have heard about it from the field. My take on it is that pg_ctl restart must be practically unused. Given that we now know it's completely broken, the only way that patching it could make the situation worse would be if the patch affected some other code path that people actually do use. As long as you're sure it doesn't do that, I see no downside to an attempted fix, even if it fails. OK, done; backpatched from 8.0.X to 8.3.X. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
[PATCHES] Fix pg_ctl restart bug
We document 'pg_ctl restart' to preserve any command-line arguments
passed when the server was started:
Restarting the server is almost equivalent to stopping the
server and starting it again except that commandpg_ctl/command
saves and reuses the command line options that were passed
to the previously running instance. To restart
However, as of 2004-10-15, this has not worked. :-( The attached patch
is the one that caused the bug --- on non-Unix systems, SYSTEMQUOTE is
, meaning zero-length string. I should have seen the bug when I
applied the contributed patch in 2004.
The second attached applied patch fixes the problem.
--
Bruce Momjian [EMAIL PROTECTED]http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Index: src/backend/postmaster/postmaster.c
===
RCS file: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.433
retrieving revision 1.434
diff -c -r1.433 -r1.434
*** src/backend/postmaster/postmaster.c 14 Oct 2004 20:23:45 - 1.433
--- src/backend/postmaster/postmaster.c 15 Oct 2004 04:54:31 - 1.434
***
*** 3301,3307
fprintf(fp, %s, fullprogname);
for (i = 1; i argc; i++)
! fprintf(fp, '%s', argv[i]);
fputs(\n, fp);
if (fclose(fp))
--- 3301,3307
fprintf(fp, %s, fullprogname);
for (i = 1; i argc; i++)
! fprintf(fp, %s%s%s, SYSTEMQUOTE, argv[i], SYSTEMQUOTE);
fputs(\n, fp);
if (fclose(fp))
Index: src/bin/pg_ctl/pg_ctl.c
===
RCS file: /cvsroot/pgsql/src/bin/pg_ctl/pg_ctl.c,v
retrieving revision 1.37
retrieving revision 1.38
diff -c -r1.37 -r1.38
*** src/bin/pg_ctl/pg_ctl.c 15 Oct 2004 04:32:14 - 1.37
--- src/bin/pg_ctl/pg_ctl.c 15 Oct 2004 04:54:33 - 1.38
***
*** 501,507
{
char *arg1;
! arg1 = strchr(optline, '\'');
if (arg1 == NULL || arg1 == optline)
post_opts = ;
else
--- 501,507
{
char *arg1;
! arg1 = strchr(optline, *SYSTEMQUOTE);
if (arg1 == NULL || arg1 == optline)
post_opts = ;
else
Index: src/backend/postmaster/postmaster.c
===
RCS file: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.560
diff -c -c -r1.560 postmaster.c
*** src/backend/postmaster/postmaster.c 26 Jun 2008 01:35:45 - 1.560
--- src/backend/postmaster/postmaster.c 26 Jun 2008 02:41:04 -
***
*** 4184,4190
fprintf(fp, %s, fullprogname);
for (i = 1; i argc; i++)
! fprintf(fp, SYSTEMQUOTE %s SYSTEMQUOTE, argv[i]);
fputs(\n, fp);
if (fclose(fp))
--- 4184,4190
fprintf(fp, %s, fullprogname);
for (i = 1; i argc; i++)
! fprintf(fp, \%s\, argv[i]);
fputs(\n, fp);
if (fclose(fp))
Index: src/bin/pg_ctl/pg_ctl.c
===
RCS file: /cvsroot/pgsql/src/bin/pg_ctl/pg_ctl.c,v
retrieving revision 1.100
diff -c -c -r1.100 pg_ctl.c
*** src/bin/pg_ctl/pg_ctl.c 26 Jun 2008 01:35:45 - 1.100
--- src/bin/pg_ctl/pg_ctl.c 26 Jun 2008 02:41:04 -
***
*** 573,583
{
if (post_opts == NULL)
{
- char **optlines;
-
post_opts = ; /* defatult */
if (ctl_command == RESTART_COMMAND)
{
optlines = readfile(postopts_file);
if (optlines == NULL)
{
--- 573,583
{
if (post_opts == NULL)
{
post_opts = ; /* defatult */
if (ctl_command == RESTART_COMMAND)
{
+ char **optlines;
+
optlines = readfile(postopts_file);
if (optlines == NULL)
{
***
*** 593,612
else
{
int len;
! char *optline = NULL;
char *arg1;
optline = optlines[0];
len = strcspn(optline, \r\n);
optline[len] = '\0';
! arg1 = strchr(optline, *SYSTEMQUOTE);
! if (arg1 == NULL || arg1 == optline)
! post_opts = ;
! else
{
! *(arg1 - 1) = '\0'; /* this should be a space */
! post_opts = arg1;
}
if (postgres_path != NULL)
postgres_path = optline;
--- 593,618
else
{
int len;
! char *optline;
char *arg1;
optline = optlines[0];
+ /* trim off line endings */
len = strcspn(optline, \r\n);
optline[len] = '\0';
! for (arg1 = optline; *arg1; arg1++)
{
! /*
! * Are we at the first option, as defined by space,
! * double-quote, and a dash?
! */
! if (*arg1 == ' ' *(arg1+1) == '' *(arg1+2) == '-')
! {
! *arg1 = '\0'; /* terminate so we get only program name */
! post_opts = arg1 + 1; /* point past whitespace */
! break;
! }
}
if (postgres_path != NULL)
postgres_path =
Re: [PATCHES] Fix pg_ctl restart bug
Bruce Momjian [EMAIL PROTECTED] writes: However, as of 2004-10-15, this has not worked. :-( The attached patch is the one that caused the bug --- on non-Unix systems, SYSTEMQUOTE is , meaning zero-length string. I should have seen the bug when I applied the contributed patch in 2004. So, shouldn't this fix be back-patched? regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix pg_ctl restart bug
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: However, as of 2004-10-15, this has not worked. :-( The attached patch is the one that caused the bug --- on non-Unix systems, SYSTEMQUOTE is , meaning zero-length string. I should have seen the bug when I applied the contributed patch in 2004. So, shouldn't this fix be back-patched? Well, no one has actually complained about the breakage, and it has been a few years. Also there is always the risk of a new bug being introduced, so I am unsure. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
