Fix pg_ctl restart bug

Bruce Momjian <bruce@momjian.us> 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

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> 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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> 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 Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:

Bruce Momjian wrote:

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> 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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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 Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> 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.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> 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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +