Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Michael Paquier]

On Wed, Aug 24, 2016 at 9:55 AM, Alvaro Herrera
 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.
>
> https://wiki.postgresql.org/wiki/Todo

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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Alvaro Herrera]

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 Herrerahttp://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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Michael Paquier]

On Mon, Aug 22, 2016 at 8:45 PM, Tom Lane  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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Tom Lane]

Michael Paquier  writes:
> On Sun, Aug 21, 2016 at 3:02 AM, Tom Lane  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.1471713...@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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Michael Paquier]

On Sun, Aug 21, 2016 at 3:02 AM, Tom Lane  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.1471713...@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
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 

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

>
> 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.


> regards, tom lane
>

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Tom Lane]

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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Tom Lane]

Michael Paquier  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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

On Sat, Aug 20, 2016 at 8:26 AM, Michael Paquier 
wrote:

> On Sat, Aug 20, 2016 at 4:39 AM, Ryan Murphy 
> 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.
>
>
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.

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Michael Paquier]

On Sat, Aug 20, 2016 at 4:39 AM, Ryan Murphy  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


initdb-path-v4.patch
Description: application/download

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

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  wrote:

>
>
> On Thu, Aug 18, 2016 at 3:44 PM, Tom Lane  wrote:
>
>> Ryan Murphy  writes:
>> > On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas 
>> 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
>>
>
> 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
> 
> links 
> about the style convention and will make sure my patch fits the conventions
> before submitting it.
>
>


0001-initdb-quote-shell-args-in-final-pg_ctl-command.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

On Thu, Aug 18, 2016 at 3:44 PM, Tom Lane  wrote:

> Ryan Murphy  writes:
> > On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas 
> 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
>

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

links 
about the style convention and will make sure my patch fits the conventions
before submitting it.

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Tom Lane]

Ryan Murphy  writes:
> On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas  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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

On Thu, Aug 18, 2016 at 3:22 PM, Robert Haas  wrote:

> On Thu, Aug 18, 2016 at 4:15 PM, Andres Freund  wrote:
> > On 2016-08-18 16:11:27 -0400, Robert Haas wrote:
> >> On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund 
> wrote:
> >> > On August 17, 2016 8:15:56 PM PDT, Michael Paquier <
> michael.paqu...@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?


> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Robert Haas]

On Thu, Aug 18, 2016 at 4:15 PM, Andres Freund  wrote:
> On 2016-08-18 16:11:27 -0400, Robert Haas wrote:
>> On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund  wrote:
>> > On August 17, 2016 8:15:56 PM PDT, Michael Paquier 
>> >  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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Andres Freund]

On 2016-08-18 16:11:27 -0400, Robert Haas wrote:
> On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund  wrote:
> > On August 17, 2016 8:15:56 PM PDT, Michael Paquier 
> >  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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Robert Haas]

On Wed, Aug 17, 2016 at 11:18 PM, Andres Freund  wrote:
> On August 17, 2016 8:15:56 PM PDT, Michael Paquier 
>  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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Andres Freund]

On August 17, 2016 8:15:56 PM PDT, Michael Paquier  
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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Michael Paquier]

On Thu, Aug 18, 2016 at 11:35 AM, Ryan Murphy  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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

> 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  wrote:

> On 2016-08-18 09:14:44 +0900, Michael Paquier wrote:
> > On Thu, Aug 18, 2016 at 12:21 AM, Ryan Murphy 
> 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.
>

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Andres Freund]

On 2016-08-18 09:14:44 +0900, Michael Paquier wrote:
> On Thu, Aug 18, 2016 at 12:21 AM, Ryan Murphy  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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Michael Paquier]

On Thu, Aug 18, 2016 at 9:30 AM, Ryan Murphy  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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

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 
wrote:

> On Thu, Aug 18, 2016 at 12:21 AM, Ryan Murphy  > 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
>

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Michael Paquier]

On Thu, Aug 18, 2016 at 12:21 AM, Ryan Murphy  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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

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  wrote:

> 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.paqu...@gmail.com> wrote:
>
>> On Wed, Aug 17, 2016 at 8:05 AM, Andres Freund 
>> 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
>>
>
>


0001-initdb-quote-shell-args-in-final-pg_ctl-command.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

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  wrote:

> On Wed, Aug 17, 2016 at 8:05 AM, Andres Freund  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
>

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Michael Paquier]

On Wed, Aug 17, 2016 at 8:05 AM, Andres Freund  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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Andres Freund]

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 
> 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   "\""

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

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

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  wrote:

> Ok, I'll do that!
> Thanks Michael!
> Ryan
>
>
> On Monday, August 15, 2016, Michael Paquier 
> wrote:
>
>> On Mon, Aug 15, 2016 at 12:12 AM, Ryan Murphy 
>> 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 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
>>
>> You may want to register this patch to the next commit fest:
>> https://commitfest.postgresql.org/10/
>> --
>> Michael
>>
>

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

Ok, I'll do that!
Thanks Michael!
Ryan

On Monday, August 15, 2016, Michael Paquier 
wrote:

> On Mon, Aug 15, 2016 at 12:12 AM, Ryan Murphy  > 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 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
>
> You may want to register this patch to the next commit fest:
> https://commitfest.postgresql.org/10/
> --
> Michael
>

Re: [HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Michael Paquier]

On Mon, Aug 15, 2016 at 12:12 AM, Ryan Murphy  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 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

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

[HACKERS] Patch: initdb: "'" for QUOTE_PATH (non-windows)

[Ryan Murphy]

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


0001-initdb-for-QUOTE_PATH-non-windows.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers