Patch: initdb: "'" for QUOTE_PATH (non-windows)
Hello Postgres Team!
This is to fix an issue that came up for me when running initdb.
At the end of a successful initdb it says:
Success. You can now start the database server using:
pg_ctl -D /some/path/to/data -l logfile start
but this command did not work for me because my data directory
contained a space. The src/bin/initdb/initdb.c source code
did already have a QUOTE_PATH constant, but it was the empty
string for non-windows cases.
Therefore, added quotes via existing QUOTE_PATH constant:
pg_ctl -D '/some/path/to/data' -l logfile start
Attachments:
0001-initdb-for-QUOTE_PATH-non-windows.patchapplication/octet-stream; name=0001-initdb-for-QUOTE_PATH-non-windows.patchDownload
From 275d045bc41b136df8c413eedba12fbd21609de1 Mon Sep 17 00:00:00 2001
From: ryanfmurphy <ryanfmurphy@gmail.com>
Date: Sun, 14 Aug 2016 08:56:50 -0500
Subject: [PATCH] initdb: "'" for QUOTE_PATH (non-windows)
fix issue when running initdb
at the end of a successful initdb it says:
Success. You can now start the database server using:
pg_ctl -D /some/path/to/data -l logfile start
but this command will not work if the data directory contains a space
therefore, added quotes via existing QUOTE_PATH constant:
pg_ctl -D '/some/path/to/data' -l logfile start
---
src/bin/initdb/initdb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 73cb7ee..6dc1e23 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -332,7 +332,7 @@ do { \
} while (0)
#ifndef WIN32
-#define QUOTE_PATH ""
+#define QUOTE_PATH "'"
#define DIR_SEP "/"
#else
#define QUOTE_PATH "\""
--
2.5.4 (Apple Git-61)
On Mon, Aug 15, 2016 at 12:12 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:
This is to fix an issue that came up for me when running initdb.
At the end of a successful initdb it says:
Success. You can now start the database server using:
pg_ctl -D /some/path/to/data -l logfile startbut this command did not work for me because my data directory
contained a space. The src/bin/initdb/initdb.c source code
did already have a QUOTE_PATH constant, but it was the empty
string for non-windows cases.Therefore, added quotes via existing QUOTE_PATH constant:
pg_ctl -D '/some/path/to/data' -l logfile start
You may want to register this patch to the next commit fest:
https://commitfest.postgresql.org/10/
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Ok, I'll do that!
Thanks Michael!
Ryan
On Monday, August 15, 2016, Michael Paquier <michael.paquier@gmail.com>
wrote:
Show quoted text
On Mon, Aug 15, 2016 at 12:12 AM, Ryan Murphy <ryanfmurphy@gmail.com
<javascript:;>> wrote:This is to fix an issue that came up for me when running initdb.
At the end of a successful initdb it says:
Success. You can now start the database server using:
pg_ctl -D /some/path/to/data -l logfile startbut this command did not work for me because my data directory
contained a space. The src/bin/initdb/initdb.c source code
did already have a QUOTE_PATH constant, but it was the empty
string for non-windows cases.Therefore, added quotes via existing QUOTE_PATH constant:
pg_ctl -D '/some/path/to/data' -l logfile start
You may want to register this patch to the next commit fest:
https://commitfest.postgresql.org/10/
--
Michael
I've submitted my patch to Commitfest 2016-09.
https://commitfest.postgresql.org/10/718/
My username on postgresql.org is murftown
On Tue, Aug 16, 2016 at 1:02 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:
Show quoted text
Ok, I'll do that!
Thanks Michael!
RyanOn Monday, August 15, 2016, Michael Paquier <michael.paquier@gmail.com>
wrote:On Mon, Aug 15, 2016 at 12:12 AM, Ryan Murphy <ryanfmurphy@gmail.com>
wrote:This is to fix an issue that came up for me when running initdb.
At the end of a successful initdb it says:
Success. You can now start the database server using:
pg_ctl -D /some/path/to/data -l logfile startbut this command did not work for me because my data directory
contained a space. The src/bin/initdb/initdb.c source code
did already have a QUOTE_PATH constant, but it was the empty
string for non-windows cases.Therefore, added quotes via existing QUOTE_PATH constant:
pg_ctl -D '/some/path/to/data' -l logfile start
You may want to register this patch to the next commit fest:
https://commitfest.postgresql.org/10/
--
Michael
Hi,
On 2016-08-14 10:12:45 -0500, Ryan Murphy wrote:
Hello Postgres Team!
but this command did not work for me because my data directory
contained a space. The src/bin/initdb/initdb.c source code
did already have a QUOTE_PATH constant, but it was the empty
string for non-windows cases.Therefore, added quotes via existing QUOTE_PATH constant:
pg_ctl -D '/some/path/to/data' -l logfile start
From 275d045bc41b136df8c413eedba12fbd21609de1 Mon Sep 17 00:00:00 2001
From: ryanfmurphy <ryanfmurphy@gmail.com>
Date: Sun, 14 Aug 2016 08:56:50 -0500
Subject: [PATCH] initdb: "'" for QUOTE_PATH (non-windows)fix issue when running initdb
at the end of a successful initdb it says:
Success. You can now start the database server using:
pg_ctl -D /some/path/to/data -l logfile startbut this command will not work if the data directory contains a space
therefore, added quotes via existing QUOTE_PATH constant:pg_ctl -D '/some/path/to/data' -l logfile start
---
src/bin/initdb/initdb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index 73cb7ee..6dc1e23 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -332,7 +332,7 @@ do { \ } while (0)#ifndef WIN32
-#define QUOTE_PATH ""
+#define QUOTE_PATH "'"
#define DIR_SEP "/"
#else
#define QUOTE_PATH "\""
ISTM that the correct fix would be to actually introduce something like
quote_path_for_shell() which either adds proper quotes, or fails if
that'd be hard (e.g. if the path contains quotes, and we're on
windows).
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 17, 2016 at 8:05 AM, Andres Freund <andres@anarazel.de> wrote:
ISTM that the correct fix would be to actually introduce something like
quote_path_for_shell() which either adds proper quotes, or fails if
that'd be hard (e.g. if the path contains quotes, and we're on
windows).
You are looking for appendShellString in fe_utils/string_utils.c.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
That makes sense, Michael and Andres.
I started to make a solution that uses a PQExpBuffer, appendShellString,
etc. I think it will work just fine, but I think I need to alter the
Makefile as well, to get initdb.c to be compiled using
-L../../../src/fe_utils -lpgfeutils. Otherwise I am having issues linking:
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv
-Wno-unused-command-line-argument -O2 initdb.o findtimezone.o localtime.o
encnames.o -L../../../src/port -L../../../src/common
-Wl,-dead_strip_dylibs -lpgcommon -lpgport -lz -lreadline -lm -o initdb
Undefined symbols for architecture x86_64:
"_appendPQExpBufferStr", referenced from:
_main in initdb.o
"_appendShellString", referenced from:
_main in initdb.o
"_createPQExpBuffer", referenced from:
_main in initdb.o
"_destroyPQExpBuffer", referenced from:
_main in initdb.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see
invocation)
On Tue, Aug 16, 2016 at 10:00 PM, Michael Paquier <michael.paquier@gmail.com
Show quoted text
wrote:
On Wed, Aug 17, 2016 at 8:05 AM, Andres Freund <andres@anarazel.de> wrote:
ISTM that the correct fix would be to actually introduce something like
quote_path_for_shell() which either adds proper quotes, or fails if
that'd be hard (e.g. if the path contains quotes, and we're on
windows).You are looking for appendShellString in fe_utils/string_utils.c.
--
Michael
I have created a better patch (attached) that correctly escapes the shell
arguments using PQExpBufferStr and the appendShellString function, as per
Michael and Andres' suggestions.
Further suggestions welcome of course.
Ryan
On Wed, Aug 17, 2016 at 8:28 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:
Show quoted text
That makes sense, Michael and Andres.
I started to make a solution that uses a PQExpBuffer, appendShellString,
etc. I think it will work just fine, but I think I need to alter the
Makefile as well, to get initdb.c to be compiled using
-L../../../src/fe_utils -lpgfeutils. Otherwise I am having issues linking:gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument
-O2 initdb.o findtimezone.o localtime.o encnames.o -L../../../src/port
-L../../../src/common -Wl,-dead_strip_dylibs -lpgcommon -lpgport -lz
-lreadline -lm -o initdb
Undefined symbols for architecture x86_64:
"_appendPQExpBufferStr", referenced from:
_main in initdb.o
"_appendShellString", referenced from:
_main in initdb.o
"_createPQExpBuffer", referenced from:
_main in initdb.o
"_destroyPQExpBuffer", referenced from:
_main in initdb.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see
invocation)On Tue, Aug 16, 2016 at 10:00 PM, Michael Paquier <
michael.paquier@gmail.com> wrote:On Wed, Aug 17, 2016 at 8:05 AM, Andres Freund <andres@anarazel.de>
wrote:ISTM that the correct fix would be to actually introduce something like
quote_path_for_shell() which either adds proper quotes, or fails if
that'd be hard (e.g. if the path contains quotes, and we're on
windows).You are looking for appendShellString in fe_utils/string_utils.c.
--
Michael
Attachments:
0001-initdb-quote-shell-args-in-final-pg_ctl-command.patchapplication/octet-stream; name=0001-initdb-quote-shell-args-in-final-pg_ctl-command.patchDownload
From d4b3d9468a3e183f0906e051b3dbc08d5b3e6bca Mon Sep 17 00:00:00 2001
From: ryanfmurphy <ryanfmurphy@gmail.com>
Date: Wed, 17 Aug 2016 10:19:12 -0500
Subject: [PATCH] initdb: quote shell args in final pg_ctl command
fix issue when running initdb
at the end of a successful initdb it says:
Success. You can now start the database server using:
/path/to/pg_ctl -D /some/path/to/data -l logfile start
but this command will not work if the data directory contains a space
therefore, correctly escape shell arguments using appendShellString
'/path/to/pg_ctl' -D '/some/path/to/data' -l logfile start
---
src/bin/initdb/Makefile | 3 +++
src/bin/initdb/initdb.c | 40 ++++++++++++++++++++++++++++++++++------
2 files changed, 37 insertions(+), 6 deletions(-)
diff --git a/src/bin/initdb/Makefile b/src/bin/initdb/Makefile
index 094c894..40aad66 100644
--- a/src/bin/initdb/Makefile
+++ b/src/bin/initdb/Makefile
@@ -16,6 +16,9 @@ subdir = src/bin/initdb
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+# need fe_utils and lpq for PQExpBuffer for quoted shell string
+LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
+
override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) -I$(top_srcdir)/src/timezone $(CPPFLAGS)
# use system timezone data?
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 73cb7ee..d6317e4 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -67,6 +67,7 @@
#include "getaddrinfo.h"
#include "getopt_long.h"
#include "miscadmin.h"
+#include "fe_utils/string_utils.h"
/* Define PG_FLUSH_DATA_WORKS if we have an implementation for pg_flush_data */
@@ -332,10 +333,8 @@ do { \
} while (0)
#ifndef WIN32
-#define QUOTE_PATH ""
#define DIR_SEP "/"
#else
-#define QUOTE_PATH "\""
#define DIR_SEP "\\"
#endif
@@ -3585,10 +3584,39 @@ main(int argc, char *argv[])
strlcpy(bin_dir, argv[0], sizeof(bin_dir));
get_parent_directory(bin_dir);
- printf(_("\nSuccess. You can now start the database server using:\n\n"
- " %s%s%spg_ctl%s -D %s%s%s -l logfile start\n\n"),
- QUOTE_PATH, bin_dir, (strlen(bin_dir) > 0) ? DIR_SEP : "", QUOTE_PATH,
- QUOTE_PATH, pgdata_native, QUOTE_PATH);
+ /*
+ * Build up a shell command to tell the user how to start the server
+ * e.g.
+ * '/path/to/pg_ctl' -D '/path/to/data_dir' -l logfile start
+ */
+ {
+ PQExpBuffer start_db_cmd = createPQExpBuffer();
+
+ { /* pg_ctl command w path, properly quoted */
+ PQExpBuffer pg_ctl_path = createPQExpBuffer();
+ printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
+ bin_dir,
+ (strlen(bin_dir) > 0) ? DIR_SEP : ""
+ );
+ appendShellString(start_db_cmd, pg_ctl_path->data);
+ destroyPQExpBuffer(pg_ctl_path);
+ }
+
+ appendPQExpBufferStr(start_db_cmd, " -D ");
+
+ /* data dir, properly quoted */
+ appendShellString(start_db_cmd, pgdata_native);
+
+ appendPQExpBufferStr(start_db_cmd, " -l logfile");
+
+ appendPQExpBufferStr(start_db_cmd, " start");
+
+ printf(_("\nSuccess. You can now start the database server using:\n\n"
+ " %s\n\n"),
+ start_db_cmd->data);
+
+ destroyPQExpBuffer(start_db_cmd);
+ }
return 0;
}
--
2.5.4 (Apple Git-61)
On Thu, Aug 18, 2016 at 12:21 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:
I have created a better patch (attached) that correctly escapes the shell
arguments using PQExpBufferStr and the appendShellString function, as per
Michael and Andres' suggestions.Further suggestions welcome of course.
As far as I know, it is perfectly possible to have LF/CR in a path
name (that's bad practice btw...), and your patch would make initdb
fail in such cases. Do we want to authorize that? If we bypass the
error checks in appendShellString with an extra option, and have
initdb use that, the generated command would be actually correct.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
That's a fair point Michael. I would be willing to make such a change, but
since c doesn't have optional function arguments I'm not sure the least
intrusive way to do that. Do you have a suggestion?
On Wednesday, August 17, 2016, Michael Paquier <michael.paquier@gmail.com>
wrote:
Show quoted text
On Thu, Aug 18, 2016 at 12:21 AM, Ryan Murphy <ryanfmurphy@gmail.com
<javascript:;>> wrote:I have created a better patch (attached) that correctly escapes the shell
arguments using PQExpBufferStr and the appendShellString function, as per
Michael and Andres' suggestions.Further suggestions welcome of course.
As far as I know, it is perfectly possible to have LF/CR in a path
name (that's bad practice btw...), and your patch would make initdb
fail in such cases. Do we want to authorize that? If we bypass the
error checks in appendShellString with an extra option, and have
initdb use that, the generated command would be actually correct.
--
Michael
On Thu, Aug 18, 2016 at 9:30 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:
(please avoid top-posting)
As far as I know, it is perfectly possible to have LF/CR in a path
name (that's bad practice btw...), and your patch would make initdb
fail in such cases. Do we want to authorize that? If we bypass the
error checks in appendShellString with an extra option, and have
initdb use that, the generated command would be actually correct.That's a fair point Michael. I would be willing to make such a change, but
since c doesn't have optional function arguments I'm not sure the least
intrusive way to do that. Do you have a suggestion?
You could just add a boolean flag to appendShellString called for
example no_error that the code path of initdb sets to true, and the
other ones to false.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-08-18 09:14:44 +0900, Michael Paquier wrote:
On Thu, Aug 18, 2016 at 12:21 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:
I have created a better patch (attached) that correctly escapes the shell
arguments using PQExpBufferStr and the appendShellString function, as per
Michael and Andres' suggestions.Further suggestions welcome of course.
As far as I know, it is perfectly possible to have LF/CR in a path
name (that's bad practice btw...), and your patch would make initdb
fail in such cases. Do we want to authorize that?
I think that's actually a good thing to forbid.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I think that's actually a good thing to forbid.
I think I agree Andres, there are already comments in the appendShellString
function to this effect - they say that CR/LF chars in a file name are
mostly used for malicious hacking attempts anyways - I know I've hardly
ever needed a newline in a file name.
Did you see anything else in my code that you have recommendations about?
I made sure to free the PQExpBufferStr vars that I allocated.
Best,
Ryan
On Wed, Aug 17, 2016 at 7:41 PM, Andres Freund <andres@anarazel.de> wrote:
Show quoted text
On 2016-08-18 09:14:44 +0900, Michael Paquier wrote:
On Thu, Aug 18, 2016 at 12:21 AM, Ryan Murphy <ryanfmurphy@gmail.com>
wrote:
I have created a better patch (attached) that correctly escapes the
shell
arguments using PQExpBufferStr and the appendShellString function, as
per
Michael and Andres' suggestions.
Further suggestions welcome of course.
As far as I know, it is perfectly possible to have LF/CR in a path
name (that's bad practice btw...), and your patch would make initdb
fail in such cases. Do we want to authorize that?I think that's actually a good thing to forbid.
On Thu, Aug 18, 2016 at 11:35 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:
Be careful of top-posting, this is not this ML style:
http://www.idallen.com/topposting.html
I think that's actually a good thing to forbid.
I think I agree Andres, there are already comments in the appendShellString
function to this effect - they say that CR/LF chars in a file name are
mostly used for malicious hacking attempts anyways - I know I've hardly ever
needed a newline in a file name.Did you see anything else in my code that you have recommendations about? I
made sure to free the PQExpBufferStr vars that I allocated.
+ { /* pg_ctl command w path, properly quoted */
+ PQExpBuffer pg_ctl_path = createPQExpBuffer();
+ printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
+ bin_dir,
+ (strlen(bin_dir) > 0) ? DIR_SEP : ""
+ );
+ appendShellString(start_db_cmd, pg_ctl_path->data);
+ destroyPQExpBuffer(pg_ctl_path);
+ }This is not really project-style to have an independent block. Usually
those are controlled by for, while or if. And you could use the same
PQExpBuffer for everything.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On August 17, 2016 8:15:56 PM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:
+ { /* pg_ctl command w path, properly quoted */ + PQExpBuffer pg_ctl_path = createPQExpBuffer(); + printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl", + bin_dir, + (strlen(bin_dir) > 0) ? DIR_SEP : "" + ); + appendShellString(start_db_cmd, pg_ctl_path->data); + destroyPQExpBuffer(pg_ctl_path); + }This is not really project-style to have an independent block. Usually
those are controlled by for, while or if.
Besides the comment positioning I'd not say that that is against the usual style, there's a number of such blocks already. Don't think it's necessarily needed here though...
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund <andres@anarazel.de> wrote:
On August 17, 2016 8:15:56 PM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:
+ { /* pg_ctl command w path, properly quoted */ + PQExpBuffer pg_ctl_path = createPQExpBuffer(); + printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl", + bin_dir, + (strlen(bin_dir) > 0) ? DIR_SEP : "" + ); + appendShellString(start_db_cmd, pg_ctl_path->data); + destroyPQExpBuffer(pg_ctl_path); + }This is not really project-style to have an independent block. Usually
those are controlled by for, while or if.Besides the comment positioning I'd not say that that is against the usual style, there's a number of such blocks already. Don't think it's necessarily needed here though...
Really? I'd remove such blocks before committing anything, or ask for
them to be removed, unless there were some special reason for having
them.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-08-18 16:11:27 -0400, Robert Haas wrote:
On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund <andres@anarazel.de> wrote:
On August 17, 2016 8:15:56 PM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:
+ { /* pg_ctl command w path, properly quoted */ + PQExpBuffer pg_ctl_path = createPQExpBuffer(); + printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl", + bin_dir, + (strlen(bin_dir) > 0) ? DIR_SEP : "" + ); + appendShellString(start_db_cmd, pg_ctl_path->data); + destroyPQExpBuffer(pg_ctl_path); + }This is not really project-style to have an independent block. Usually
those are controlled by for, while or if.Besides the comment positioning I'd not say that that is against the usual style, there's a number of such blocks already. Don't think it's necessarily needed here though...
Really? I'd remove such blocks before committing anything, or ask for
them to be removed, unless there were some special reason for having
them.
Well, reducing the scope of variables *can* be such a reason, no? As I
said, I don't see any reason here, but in general, it's imo a reasonable
tool on one's belt.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Aug 18, 2016 at 4:15 PM, Andres Freund <andres@anarazel.de> wrote:
On 2016-08-18 16:11:27 -0400, Robert Haas wrote:
On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund <andres@anarazel.de> wrote:
On August 17, 2016 8:15:56 PM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:
+ { /* pg_ctl command w path, properly quoted */ + PQExpBuffer pg_ctl_path = createPQExpBuffer(); + printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl", + bin_dir, + (strlen(bin_dir) > 0) ? DIR_SEP : "" + ); + appendShellString(start_db_cmd, pg_ctl_path->data); + destroyPQExpBuffer(pg_ctl_path); + }This is not really project-style to have an independent block. Usually
those are controlled by for, while or if.Besides the comment positioning I'd not say that that is against the usual style, there's a number of such blocks already. Don't think it's necessarily needed here though...
Really? I'd remove such blocks before committing anything, or ask for
them to be removed, unless there were some special reason for having
them.Well, reducing the scope of variables *can* be such a reason, no? As I
said, I don't see any reason here, but in general, it's imo a reasonable
tool on one's belt.
I think it's worth reducing the scope of variables when that's as
simple as putting them inside a block that you have to create anyway,
but I'm skeptical about the idea that one would create a block just to
reduce the scope of the variables. I don't think that's our usual
practice, and I would expect the compiler to detect where the variable
is referenced first and last anyway.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Aug 18, 2016 at 4:15 PM, Andres Freund <andres@anarazel.de> wrote:
On 2016-08-18 16:11:27 -0400, Robert Haas wrote:
On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund <andres@anarazel.de>
wrote:
On August 17, 2016 8:15:56 PM PDT, Michael Paquier <
michael.paquier@gmail.com> wrote:
+ { /* pg_ctl command w path, properly quoted */ + PQExpBuffer pg_ctl_path = createPQExpBuffer(); + printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl", + bin_dir, + (strlen(bin_dir) > 0) ? DIR_SEP : "" + ); + appendShellString(start_db_cmd, pg_ctl_path->data); + destroyPQExpBuffer(pg_ctl_path); + }This is not really project-style to have an independent block. Usually
those are controlled by for, while or if.Besides the comment positioning I'd not say that that is against the
usual style, there's a number of such blocks already. Don't think it's
necessarily needed here though...Really? I'd remove such blocks before committing anything, or ask for
them to be removed, unless there were some special reason for having
them.Well, reducing the scope of variables *can* be such a reason, no? As I
said, I don't see any reason here, but in general, it's imo a reasonable
tool on one's belt.I think it's worth reducing the scope of variables when that's as
simple as putting them inside a block that you have to create anyway,
but I'm skeptical about the idea that one would create a block just to
reduce the scope of the variables. I don't think that's our usual
practice, and I would expect the compiler to detect where the variable
is referenced first and last anyway.
I'm can change my patch to take out that block.
I enjoy adding the blocks for explicit variable scoping and for quick
navigation in vim (the % key navigates between matching {}'s). But I want
to fit in with the style conventions of the project.
Before I change and resubmit my patch, are there any other changes, style
or otherwise, that you all would recommend?
Show quoted text
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Ryan Murphy <ryanfmurphy@gmail.com> writes:
On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas <robertmhaas@gmail.com> wrote:
+ { /* pg_ctl command w path, properly quoted */ + PQExpBuffer pg_ctl_path = createPQExpBuffer(); + printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
I think it's worth reducing the scope of variables when that's as
simple as putting them inside a block that you have to create anyway,
but I'm skeptical about the idea that one would create a block just to
reduce the scope of the variables. I don't think that's our usual
practice, and I would expect the compiler to detect where the variable
is referenced first and last anyway.
I enjoy adding the blocks for explicit variable scoping and for quick
navigation in vim (the % key navigates between matching {}'s). But I want
to fit in with the style conventions of the project.
Another point here is that code like this will look quite a bit different
after pgindent gets done with it --- that comment will not stay where
you put it, for example. Some of our worst formatting messes come from
code wherein somebody adhered to their own favorite layout style without
any thought for how it would play with pgindent.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Aug 18, 2016 at 3:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Ryan Murphy <ryanfmurphy@gmail.com> writes:
On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas <robertmhaas@gmail.com>
wrote:
+ { /* pg_ctl command w path, properly quoted */ + PQExpBuffer pg_ctl_path = createPQExpBuffer(); + printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",I think it's worth reducing the scope of variables when that's as
simple as putting them inside a block that you have to create anyway,
but I'm skeptical about the idea that one would create a block just to
reduce the scope of the variables. I don't think that's our usual
practice, and I would expect the compiler to detect where the variable
is referenced first and last anyway.I enjoy adding the blocks for explicit variable scoping and for quick
navigation in vim (the % key navigates between matching {}'s). But Iwant
to fit in with the style conventions of the project.
Another point here is that code like this will look quite a bit different
after pgindent gets done with it --- that comment will not stay where
you put it, for example. Some of our worst formatting messes come from
code wherein somebody adhered to their own favorite layout style without
any thought for how it would play with pgindent.regards, tom lane
Ahh, I didn't know about pgindent, but now I see it in /src/tools. I can
run that on my code before submitting.
I found these
</messages/by-id/1221125165.5637.12.camel@abbas-laptop>
links <https://www.postgresql.org/docs/devel/static/source-format.html>
about the style convention and will make sure my patch fits the conventions
before submitting it.
Here is another version of my initdb shell quoting patch. I have removed
the unnecessary {} block. I also ran pgindent on the code prior to
creating the patch.
On Thu, Aug 18, 2016 at 3:50 PM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:
Show quoted text
On Thu, Aug 18, 2016 at 3:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Ryan Murphy <ryanfmurphy@gmail.com> writes:
On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas <robertmhaas@gmail.com>
wrote:
+ { /* pg_ctl command w path, properly quoted */ + PQExpBuffer pg_ctl_path = createPQExpBuffer(); + printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",I think it's worth reducing the scope of variables when that's as
simple as putting them inside a block that you have to create anyway,
but I'm skeptical about the idea that one would create a block just to
reduce the scope of the variables. I don't think that's our usual
practice, and I would expect the compiler to detect where the variable
is referenced first and last anyway.I enjoy adding the blocks for explicit variable scoping and for quick
navigation in vim (the % key navigates between matching {}'s). But Iwant
to fit in with the style conventions of the project.
Another point here is that code like this will look quite a bit different
after pgindent gets done with it --- that comment will not stay where
you put it, for example. Some of our worst formatting messes come from
code wherein somebody adhered to their own favorite layout style without
any thought for how it would play with pgindent.regards, tom lane
Ahh, I didn't know about pgindent, but now I see it in /src/tools. I can
run that on my code before submitting.I found these
</messages/by-id/1221125165.5637.12.camel@abbas-laptop>
links <https://www.postgresql.org/docs/devel/static/source-format.html>
about the style convention and will make sure my patch fits the conventions
before submitting it.
Attachments:
0001-initdb-quote-shell-args-in-final-pg_ctl-command.patchapplication/octet-stream; name=0001-initdb-quote-shell-args-in-final-pg_ctl-command.patchDownload
From 39be349405462dfb015e214386d88f81180f8426 Mon Sep 17 00:00:00 2001
From: ryanfmurphy <ryanfmurphy@gmail.com>
Date: Fri, 19 Aug 2016 14:39:26 -0500
Subject: [PATCH] initdb: quote shell args in final pg_ctl command
fix issue when running initdb
at the end of a successful initdb it says:
Success. You can now start the database server using:
/path/to/pg_ctl -D /some/path/to/data -l logfile start
but this command will not work if the data directory contains a space
therefore, correctly escape shell arguments using appendShellString
'/path/to/pg_ctl' -D '/some/path/to/data' -l logfile start
---
src/bin/initdb/Makefile | 3 +++
src/bin/initdb/initdb.c | 39 ++++++++++++++++++++++++++++++++++-----
2 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/src/bin/initdb/Makefile b/src/bin/initdb/Makefile
index 094c894..40aad66 100644
--- a/src/bin/initdb/Makefile
+++ b/src/bin/initdb/Makefile
@@ -16,6 +16,9 @@ subdir = src/bin/initdb
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+# need fe_utils and lpq for PQExpBuffer for quoted shell string
+LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
+
override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) -I$(top_srcdir)/src/timezone $(CPPFLAGS)
# use system timezone data?
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 73cb7ee..11b406d 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -67,6 +67,7 @@
#include "getaddrinfo.h"
#include "getopt_long.h"
#include "miscadmin.h"
+#include "fe_utils/string_utils.h"
/* Define PG_FLUSH_DATA_WORKS if we have an implementation for pg_flush_data */
@@ -332,10 +333,8 @@ do { \
} while (0)
#ifndef WIN32
-#define QUOTE_PATH ""
#define DIR_SEP "/"
#else
-#define QUOTE_PATH "\""
#define DIR_SEP "\\"
#endif
@@ -3585,10 +3584,40 @@ main(int argc, char *argv[])
strlcpy(bin_dir, argv[0], sizeof(bin_dir));
get_parent_directory(bin_dir);
+
+
+ /*
+ * Build up a shell command to tell the user how to start the server e.g.
+ * '/path/to/pg_ctl' -D '/path/to/data_dir' -l logfile start
+ */
+ PQExpBuffer start_db_cmd = createPQExpBuffer();
+
+ /* build pg_ctl command w path, properly quoted */
+ PQExpBuffer pg_ctl_path = createPQExpBuffer();
+
+ printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
+ bin_dir,
+ (strlen(bin_dir) > 0) ? DIR_SEP : ""
+ );
+ appendShellString(start_db_cmd, pg_ctl_path->data);
+ destroyPQExpBuffer(pg_ctl_path);
+
+ appendPQExpBufferStr(start_db_cmd, " -D ");
+
+ /* data dir, properly quoted */
+ appendShellString(start_db_cmd, pgdata_native);
+
+ appendPQExpBufferStr(start_db_cmd, " -l logfile");
+
+ appendPQExpBufferStr(start_db_cmd, " start");
+
printf(_("\nSuccess. You can now start the database server using:\n\n"
- " %s%s%spg_ctl%s -D %s%s%s -l logfile start\n\n"),
- QUOTE_PATH, bin_dir, (strlen(bin_dir) > 0) ? DIR_SEP : "", QUOTE_PATH,
- QUOTE_PATH, pgdata_native, QUOTE_PATH);
+ " %s\n\n"),
+ start_db_cmd->data);
+
+ destroyPQExpBuffer(start_db_cmd);
+
+
return 0;
}
--
2.5.4 (Apple Git-61)
On Sat, Aug 20, 2016 at 4:39 AM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:
Here is another version of my initdb shell quoting patch. I have removed
the unnecessary {} block. I also ran pgindent on the code prior to creating
the patch.
Could you please *not* top-post? This breaks the logic of the thread,
this is the third time that I mention it, and that's not the style of
this mailing list.
Regarding your patch, with a bit of clean up it gives the attached.
You should declare variables at the beginning of a code block or
function. One call to appendPQExpBufferStr can as well be avoided, and
you added too many newlines. I have switched that as ready for
committer.
--
Michael
Attachments:
initdb-path-v4.patchapplication/x-download; name=initdb-path-v4.patchDownload
diff --git a/src/bin/initdb/Makefile b/src/bin/initdb/Makefile
index 094c894..122c2e2 100644
--- a/src/bin/initdb/Makefile
+++ b/src/bin/initdb/Makefile
@@ -17,6 +17,7 @@ top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) -I$(top_srcdir)/src/timezone $(CPPFLAGS)
+LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
# use system timezone data?
ifneq (,$(with_system_tzdata))
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index a978bbc..0c2262d 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -67,6 +67,7 @@
#include "getaddrinfo.h"
#include "getopt_long.h"
#include "miscadmin.h"
+#include "fe_utils/string_utils.h"
/* Define PG_FLUSH_DATA_WORKS if we have an implementation for pg_flush_data */
@@ -332,10 +333,8 @@ do { \
} while (0)
#ifndef WIN32
-#define QUOTE_PATH ""
#define DIR_SEP "/"
#else
-#define QUOTE_PATH "\""
#define DIR_SEP "\\"
#endif
@@ -3360,6 +3359,8 @@ main(int argc, char *argv[])
int option_index;
char *effective_user;
char bin_dir[MAXPGPATH];
+ PQExpBuffer start_db_cmd;
+ PQExpBuffer pg_ctl_path;
/*
* Ensure that buffering behavior of stdout and stderr matches what it is
@@ -3591,10 +3592,33 @@ main(int argc, char *argv[])
strlcpy(bin_dir, argv[0], sizeof(bin_dir));
get_parent_directory(bin_dir);
+ /*
+ * Build up a shell command to tell the user how to start the server e.g.
+ * '/path/to/pg_ctl' -D '/path/to/data_dir' -l logfile start
+ */
+ pg_ctl_path = createPQExpBuffer();
+ start_db_cmd = createPQExpBuffer();
+
+ printfPQExpBuffer(pg_ctl_path, "%s%spg_ctl",
+ bin_dir,
+ (strlen(bin_dir) > 0) ? DIR_SEP : "");
+
+ /* path to pg_ctl, properly quoted */
+ appendShellString(start_db_cmd, pg_ctl_path->data);
+
+ appendPQExpBufferStr(start_db_cmd, " -D ");
+
+ /* and data directory, quoted as well */
+ appendShellString(start_db_cmd, pgdata_native);
+
+ appendPQExpBufferStr(start_db_cmd, " -l logfile start");
+
printf(_("\nSuccess. You can now start the database server using:\n\n"
- " %s%s%spg_ctl%s -D %s%s%s -l logfile start\n\n"),
- QUOTE_PATH, bin_dir, (strlen(bin_dir) > 0) ? DIR_SEP : "", QUOTE_PATH,
- QUOTE_PATH, pgdata_native, QUOTE_PATH);
+ " %s\n\n"),
+ start_db_cmd->data);
+
+ destroyPQExpBuffer(pg_ctl_path);
+ destroyPQExpBuffer(start_db_cmd);
return 0;
}
On Sat, Aug 20, 2016 at 8:26 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:
On Sat, Aug 20, 2016 at 4:39 AM, Ryan Murphy <ryanfmurphy@gmail.com>
wrote:Here is another version of my initdb shell quoting patch. I have removed
the unnecessary {} block. I also ran pgindent on the code prior tocreating
the patch.
Could you please *not* top-post? This breaks the logic of the thread,
this is the third time that I mention it, and that's not the style of
this mailing list.
Sure, sorry about that. Am not used to this style of mailing list. I had
been avoiding top posting since you first mentioned it but then with the
new patch I didn't know if that was still supposed to go at the bottom.
Regarding your patch, with a bit of clean up it gives the attached.
You should declare variables at the beginning of a code block or
function. One call to appendPQExpBufferStr can as well be avoided, and
you added too many newlines. I have switched that as ready for
committer.
--
Michael
Thanks Michael, I've looked at your changes and everything looks good to me.
I did notice the commit message is gone etc., is that not part of the
patch? Perhaps the committer prefers to write their own message, that's
fine too.
Michael Paquier <michael.paquier@gmail.com> writes:
Regarding your patch, with a bit of clean up it gives the attached.
This fails to build for me, with
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -g -O1 initdb.o findtimezone.o localtime.o encnames.o -L../../../src/port -L../../../src/common -Wl,--as-needed -Wl,-rpath,'/home/postgres/testversion/lib',--enable-new-dtags -L../../../src/fe_utils -lpgfeutils -lpq -lpgcommon -lpgport -lz -lreadline -lrt -lcrypt -ldl -lm -o initdb
/usr/bin/ld: cannot find -lpq
collect2: ld returned 1 exit status
make: *** [initdb] Error 1
evidently because the link command omits the necessary -L switch, because
you didn't use the approved macro for linking in libpq. It should be
$(libpq_pgport) instead, I believe. (Probably the reason it seems to work
for you is you have a version of libpq.so in /usr/lib; but then you are
really doing the link against the wrong version of libpq.)
A bigger issue here is that it seems fundamentally wrong for initdb to be
including libpq, because it surely is never meant to be communicating
with a running postmaster. Not sure what to do about that. We could
consider moving pqexpbuffer out of libpq into fe_utils, but I wonder
whether that would break any third-party code. We've never advertised
pqexpbuffer.h as a supported API of libpq, but it's probably handy enough
that people use it anyway. I suppose we could duplicate it in fe_utils
and libpq, though that's a tad ugly. Thoughts?
Another perhaps-only-cosmetic issue is that now initdb prints quotes
whether they are needed or not. I find this output pretty ugly:
Success. You can now start the database server using:
'pg_ctl' -D '/home/postgres/testversion/data' -l logfile start
That's not really the fault of this patch perhaps. Maybe we could adjust
appendShellString so it doesn't add quotes if they are clearly
unnecessary.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
A bigger issue here is that it seems fundamentally wrong for initdb to be
including libpq, because it surely is never meant to be communicating
with a running postmaster. Not sure what to do about that. We could
consider moving pqexpbuffer out of libpq into fe_utils, but I wonder
whether that would break any third-party code. We've never advertised
pqexpbuffer.h as a supported API of libpq, but it's probably handy enough
that people use it anyway. I suppose we could duplicate it in fe_utils
and libpq, though that's a tad ugly. Thoughts?
I looked into this and soon found that fe_utils/string_utils.o has
dependencies on libpq that are much wider than just pqexpbuffer :-(.
It might be a project to think about sorting that out sometime, but it
looks like it would be an awful lot of work just to avoid having initdb
depend on libpq.so. So I now think this objection is impractical.
I'll just annotate the makefile entry to say that initdb itself doesn't
use libpq.
Another perhaps-only-cosmetic issue is that now initdb prints quotes
whether they are needed or not. I find this output pretty ugly:
...
That's not really the fault of this patch perhaps. Maybe we could adjust
appendShellString so it doesn't add quotes if they are clearly
unnecessary.
I still think this is worth fixing, but it's a simple modification.
Will take care of it.
This item is listed in the commitfest as a bug fix, but given the lack of
previous complaints, and the fact that the printed command isn't really
meant to be blindly copied-and-pasted anyway (you're at least meant to
replace "logfile" with something), I do not think it needs back-patching.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I looked into this and soon found that fe_utils/string_utils.o has
dependencies on libpq that are much wider than just pqexpbuffer :-(.
It might be a project to think about sorting that out sometime, but it
looks like it would be an awful lot of work just to avoid having initdb
depend on libpq.so. So I now think this objection is impractical.
I'll just annotate the makefile entry to say that initdb itself doesn't
use libpq.
Sounds good.
Another perhaps-only-cosmetic issue is that now initdb prints quotes
whether they are needed or not.I still think this is worth fixing, but it's a simple modification.
Will take care of it.Great! Seems like a good solution, I agree it looks better without quotes
if they're not needed.
This item is listed in the commitfest as a bug fix, but given the lack of
previous complaints, and the fact that the printed command isn't really
meant to be blindly copied-and-pasted anyway (you're at least meant to
replace "logfile" with something), I do not think it needs back-patching.
Agreed, not a "bug" in that sense.
Thanks Tom.
Show quoted text
regards, tom lane
On Sun, Aug 21, 2016 at 3:02 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
A bigger issue here is that it seems fundamentally wrong for initdb to be
including libpq, because it surely is never meant to be communicating
with a running postmaster. Not sure what to do about that. We could
consider moving pqexpbuffer out of libpq into fe_utils, but I wonder
whether that would break any third-party code. We've never advertised
pqexpbuffer.h as a supported API of libpq, but it's probably handy enough
that people use it anyway. I suppose we could duplicate it in fe_utils
and libpq, though that's a tad ugly. Thoughts?I looked into this and soon found that fe_utils/string_utils.o has
dependencies on libpq that are much wider than just pqexpbuffer :-(.
It might be a project to think about sorting that out sometime, but it
looks like it would be an awful lot of work just to avoid having initdb
depend on libpq.so. So I now think this objection is impractical.
I'll just annotate the makefile entry to say that initdb itself doesn't
use libpq.
pqexpbuffer.c is an independent piece of facility, so we could move it
to src/common and leverage the dependency a bit, and have libpq link
to the source file itself at build phase. The real problem is the call
to PQmblen in psqlscan.l... And this, I am not sure how this could be
refactored cleanly.
Another perhaps-only-cosmetic issue is that now initdb prints quotes
whether they are needed or not. I find this output pretty ugly:
...
That's not really the fault of this patch perhaps. Maybe we could adjust
appendShellString so it doesn't add quotes if they are clearly
unnecessary.I still think this is worth fixing, but it's a simple modification.
Will take care of it.This item is listed in the commitfest as a bug fix, but given the lack of
previous complaints, and the fact that the printed command isn't really
meant to be blindly copied-and-pasted anyway (you're at least meant to
replace "logfile" with something), I do not think it needs back-patching.
Agreed on that. Only first-time users do a copy-paste of the command
anyway, so the impact is very narrow.
And actually, I had a look at the build failure that you reported in
3855.1471713949@sss.pgh.pa.us. While that was because of a copy of
libpq.so that I had in my own LD_LIBRARY_PATH, shouldn't all the other
frontend utilities depending on fe_utils also use $(libpq_pgport)
instead of -lpq? I think that you saw the error for initdb because
that's the first one to be built in src/bin/, and I think that Ryan
used -lpq in his patch because the same pattern is used everywhere
else.
It seems to me as well that submake-libpq and submake-libpgfeutils
should be listed in initdb's Makefile when building the binary.
Am I missing something? Please see the patch attached to see what I mean.
Thinking harder about that, it may be better to even add a variable in
Makefile.global.in for utilities in need of fe_utils, like that for
example:
libpg_feutils = -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
Thoughts?
--
Michael
Attachments:
bin-make-cleanup.patchtext/x-patch; charset=US-ASCII; name=bin-make-cleanup.patchDownload
diff --git a/src/bin/initdb/Makefile b/src/bin/initdb/Makefile
index 531cc97..394eae0 100644
--- a/src/bin/initdb/Makefile
+++ b/src/bin/initdb/Makefile
@@ -30,7 +30,7 @@ OBJS= initdb.o findtimezone.o localtime.o encnames.o $(WIN32RES)
all: initdb
-initdb: $(OBJS) | submake-libpgport
+initdb: $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
# We used to pull in all of libpq to get encnames.c, but that
diff --git a/src/bin/pg_dump/Makefile b/src/bin/pg_dump/Makefile
index 25336a5..08b01d7 100644
--- a/src/bin/pg_dump/Makefile
+++ b/src/bin/pg_dump/Makefile
@@ -17,7 +17,7 @@ top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
override CPPFLAGS := -I$(libpq_srcdir) $(CPPFLAGS)
-LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
+LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
OBJS= pg_backup_archiver.o pg_backup_db.o pg_backup_custom.o \
pg_backup_null.o pg_backup_tar.o pg_backup_directory.o \
diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile
index 8823288..aa3c45b 100644
--- a/src/bin/pg_upgrade/Makefile
+++ b/src/bin/pg_upgrade/Makefile
@@ -12,7 +12,7 @@ OBJS = check.o controldata.o dump.o exec.o file.o function.o info.o \
tablespace.o util.o version.o $(WIN32RES)
override CPPFLAGS := -DDLSUFFIX=\"$(DLSUFFIX)\" -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
-LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
+LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
all: pg_upgrade
diff --git a/src/bin/pgbench/Makefile b/src/bin/pgbench/Makefile
index 1503d00..228eed5 100644
--- a/src/bin/pgbench/Makefile
+++ b/src/bin/pgbench/Makefile
@@ -10,7 +10,7 @@ include $(top_builddir)/src/Makefile.global
OBJS = pgbench.o exprparse.o $(WIN32RES)
override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
-LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
+LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
ifneq ($(PORTNAME), win32)
override CFLAGS += $(PTHREAD_CFLAGS)
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index 1f6a289..2c727c5 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -19,7 +19,7 @@ include $(top_builddir)/src/Makefile.global
REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
-LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
+LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
OBJS= command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
diff --git a/src/bin/scripts/Makefile b/src/bin/scripts/Makefile
index 8c107b1..7f218f5 100644
--- a/src/bin/scripts/Makefile
+++ b/src/bin/scripts/Makefile
@@ -19,7 +19,7 @@ include $(top_builddir)/src/Makefile.global
PROGRAMS = createdb createlang createuser dropdb droplang dropuser clusterdb vacuumdb reindexdb pg_isready
override CPPFLAGS := -I$(libpq_srcdir) $(CPPFLAGS)
-LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
+LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
all: $(PROGRAMS)
Michael Paquier <michael.paquier@gmail.com> writes:
On Sun, Aug 21, 2016 at 3:02 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I looked into this and soon found that fe_utils/string_utils.o has
dependencies on libpq that are much wider than just pqexpbuffer :-(.
pqexpbuffer.c is an independent piece of facility, so we could move it
to src/common and leverage the dependency a bit, and have libpq link
to the source file itself at build phase. The real problem is the call
to PQmblen in psqlscan.l... And this, I am not sure how this could be
refactored cleanly.
I see all of these as libpq dependencies in string_utils.o:
U PQclientEncoding
U PQescapeStringConn
U PQmblen
U PQserverVersion
Maybe we could split that file into two parts (libpq-dependent and not)
but it would be pretty arbitrary.
And actually, I had a look at the build failure that you reported in
3855.1471713949@sss.pgh.pa.us. While that was because of a copy of
libpq.so that I had in my own LD_LIBRARY_PATH, shouldn't all the other
frontend utilities depending on fe_utils also use $(libpq_pgport)
instead of -lpq?
All the rest of them already have that, because their link commands
look like, eg for psql,
LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
psql: $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) $(OBJS) $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
^^^^^^^^^^^^^^^^^^^^^^^^^^
The extra reference to -lpq just makes sure that references to libpq from
libpgfeutils get resolved properly. (And yeah, there are some platforms
that need that :-(.) We don't need an extra copy of the -L flag.
This is all pretty messy, not least because of the way libpq_pgport
is set up; as Makefile.global notes,
# ... This does cause duplicate -lpgport's to appear
# on client link lines.
Likely it would be better to refactor all of this so we get just one
reference to libpq and one reference to libpgport, but that'd require
a more thoroughgoing cleanup than you have here. (Now that I think
about it, adding -L/-l to LDFLAGS is pretty duff coding style to
begin with --- we should be adding those things to LIBS, or at least
putting them just before LIBS in the command lines.)
You're right that I missed the desirability of invoking
submake-libpq and submake-libpgfeutils in initdb's build.
Will fix that.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Aug 22, 2016 at 8:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Likely it would be better to refactor all of this so we get just one
reference to libpq and one reference to libpgport, but that'd require
a more thoroughgoing cleanup than you have here. (Now that I think
about it, adding -L/-l to LDFLAGS is pretty duff coding style to
begin with --- we should be adding those things to LIBS, or at least
putting them just before LIBS in the command lines.)
Agreed, this needs more thoughts. As that's messy, I won't high-jack
more this thread and begin a new one with a more fully-bloomed patch
once I get time to look at it.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Thanks for committing it Tom! Thank you all for your help.
I really like the Postgres project! If there's anything that especially
needs worked on let me know, I'd love to help.
Best,
Ryan
Ryan Murphy wrote:
Thanks for committing it Tom! Thank you all for your help.
I really like the Postgres project! If there's anything that especially
needs worked on let me know, I'd love to help.
https://wiki.postgresql.org/wiki/Todo
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 24, 2016 at 9:55 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Ryan Murphy wrote:
Thanks for committing it Tom! Thank you all for your help.
I really like the Postgres project! If there's anything that especially
needs worked on let me know, I'd love to help.
Even with that, there are always patches waiting for reviews, tests or input:
https://commitfest.postgresql.org/10/
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers