Re: pgsql: Refactor flex and bison make rules

Refactor flex and bison make rules

Numerous flex and bison make rules have appeared in the source tree
over time, and they are all virtually identical, so we can replace
them by pattern rules with some variables for customization.

Users of pgxs will also be able to benefit from this.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/8521d131941be5a177270bc428fa8e684cd645b5

Modified Files
--------------
contrib/cube/Makefile | 14 --------------
contrib/seg/Makefile | 14 --------------
src/Makefile.global.in | 16 ++++++++++++++++
src/backend/bootstrap/Makefile | 16 ----------------
src/backend/parser/Makefile | 20 ++++----------------
src/backend/replication/Makefile | 14 --------------
src/backend/utils/misc/Makefile | 7 -------
src/bin/psql/Makefile | 11 ++---------
src/interfaces/ecpg/preproc/Makefile | 15 +--------------
src/pl/plpgsql/src/Makefile | 9 +--------
src/test/isolation/Makefile | 14 --------------
11 files changed, 24 insertions(+), 126 deletions(-)

Peter Eisentraut wrote:
> Refactor flex and bison make rules
>
> Numerous flex and bison make rules have appeared in the source tree
> over time, and they are all virtually identical, so we can replace
> them by pattern rules with some variables for customization.

Okapi has been failing sporadically on ecpg, and I wonder if it's
related to this change. See, for example,
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=okapi&dt=2012-11-28%2018%3A15%3A01

icc -O3 -xSSSE3 -parallel -ip -mp1 -fno-strict-aliasing -g -pthread -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS -DECPG_COMPILE -I../include -I../../../../src/interfaces/ecpg/include -I. -I. -DMAJOR_VERSION=4 -DMINOR_VERSION=9 -DPATCHLEVEL=0 -I../../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include/et -c -o parser.o parser.c
icc -O3 -xSSSE3 -parallel -ip -mp1 -fno-strict-aliasing -g -pthread -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS -DECPG_COMPILE -I../include -I../../../../src/interfaces/ecpg/include -I. -I. -DMAJOR_VERSION=4 -DMINOR_VERSION=9 -DPATCHLEVEL=0 -I../../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include/et -c -o keywords.o keywords.c
parser.c(25): catastrophic error: could not open source file "preproc.h"
#include "preproc.h"
^

compilation aborted for parser.c (code 4)
make[4]: *** [parser.o] Error 4
make[4]: *** Waiting for unfinished jobs....
keywords.c(20): catastrophic error: could not open source file "preproc.h"
#include "preproc.h"
^

compilation aborted for keywords.c (code 4)

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 11/28/2012 02:14 PM, Alvaro Herrera wrote:
> Peter Eisentraut wrote:
>> Refactor flex and bison make rules
>>
>> Numerous flex and bison make rules have appeared in the source tree
>> over time, and they are all virtually identical, so we can replace
>> them by pattern rules with some variables for customization.
> Okapi has been failing sporadically on ecpg, and I wonder if it's
> related to this change. See, for example,
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=okapi&dt=2012-11-28%2018%3A15%3A01
>
> icc -O3 -xSSSE3 -parallel -ip -mp1 -fno-strict-aliasing -g -pthread -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS -DECPG_COMPILE -I../include -I../../../../src/interfaces/ecpg/include -I. -I. -DMAJOR_VERSION=4 -DMINOR_VERSION=9 -DPATCHLEVEL=0 -I../../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include/et -c -o parser.o parser.c
> icc -O3 -xSSSE3 -parallel -ip -mp1 -fno-strict-aliasing -g -pthread -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS -DECPG_COMPILE -I../include -I../../../../src/interfaces/ecpg/include -I. -I. -DMAJOR_VERSION=4 -DMINOR_VERSION=9 -DPATCHLEVEL=0 -I../../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include/et -c -o keywords.o keywords.c
> parser.c(25): catastrophic error: could not open source file "preproc.h"
> #include "preproc.h"
> ^
>
> compilation aborted for parser.c (code 4)
> make[4]: *** [parser.o] Error 4
> make[4]: *** Waiting for unfinished jobs....
> keywords.c(20): catastrophic error: could not open source file "preproc.h"
> #include "preproc.h"
> ^
>
> compilation aborted for keywords.c (code 4)

Well, it looks like the make is broken and missing a clear dependency
requirement. I think we need to ask Jeremy to turn off parallel build
for okapi.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 11/28/2012 02:14 PM, Alvaro Herrera wrote:
>> Okapi has been failing sporadically on ecpg, and I wonder if it's
>> related to this change.

> Well, it looks like the make is broken and missing a clear dependency
> requirement. I think we need to ask Jeremy to turn off parallel build
> for okapi.

Yeah, we already know that unpatched make 3.82 has got serious
parallelism bugs:
http://archives.postgresql.org/pgsql-hackers/2012-09/msg00397.php

I wonder whether adding another .NOTPARALLEL directive would be a better
idea than insisting people get hold of patched versions.

regards, tom lane

On 11/28/12 6:01 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 11/28/2012 02:14 PM, Alvaro Herrera wrote:
>>> Okapi has been failing sporadically on ecpg, and I wonder if it's
>>> related to this change.
>
>> Well, it looks like the make is broken and missing a clear dependency
>> requirement. I think we need to ask Jeremy to turn off parallel build
>> for okapi.
>
> Yeah, we already know that unpatched make 3.82 has got serious
> parallelism bugs:
> http://archives.postgresql.org/pgsql-hackers/2012-09/msg00397.php
>
> I wonder whether adding another .NOTPARALLEL directive would be a better
> idea than insisting people get hold of patched versions.

We could put

ifeq ($(MAKE_VERSION),3.82)
.NOTPARALLEL:
endif

into Makefile.global.

On 11/28/2012 06:01 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 11/28/2012 02:14 PM, Alvaro Herrera wrote:
>>> Okapi has been failing sporadically on ecpg, and I wonder if it's
>>> related to this change.
>> Well, it looks like the make is broken and missing a clear dependency
>> requirement. I think we need to ask Jeremy to turn off parallel build
>> for okapi.
> Yeah, we already know that unpatched make 3.82 has got serious
> parallelism bugs:
> http://archives.postgresql.org/pgsql-hackers/2012-09/msg00397.php
>
> I wonder whether adding another .NOTPARALLEL directive would be a better
> idea than insisting people get hold of patched versions.
>
>

You mean in the preproc Makefile?

Maybe.

cheers

andrew

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 11/28/12 6:01 PM, Tom Lane wrote:
>> I wonder whether adding another .NOTPARALLEL directive would be a better
>> idea than insisting people get hold of patched versions.

> We could put
> ifeq ($(MAKE_VERSION),3.82)
> .NOTPARALLEL:
> endif
> into Makefile.global.

I don't wish to go *that* far. Parallel make works fine for most of the
tree in 3.82, and shutting it off would penalize developers a lot.

It appears to me that the case that okapi is hitting is specific to the
ecpg preprocessor build rules, and indeed specific to the case where
preproc.c needs to be rebuilt. A .NOTPARALLEL in ecpg/preproc/Makefile
would probably be enough to fix it. (I'm a bit tempted to make the one
already added to ecpg/Makefile conditional on the make version, as you
suggest above, too.)

regards, tom lane

On 11/28/2012 06:19 PM, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> On 11/28/12 6:01 PM, Tom Lane wrote:
>>> I wonder whether adding another .NOTPARALLEL directive would be a better
>>> idea than insisting people get hold of patched versions.
>> We could put
>> ifeq ($(MAKE_VERSION),3.82)
>> .NOTPARALLEL:
>> endif
>> into Makefile.global.
> I don't wish to go *that* far. Parallel make works fine for most of the
> tree in 3.82, and shutting it off would penalize developers a lot.
>
> It appears to me that the case that okapi is hitting is specific to the
> ecpg preprocessor build rules, and indeed specific to the case where
> preproc.c needs to be rebuilt. A .NOTPARALLEL in ecpg/preproc/Makefile
> would probably be enough to fix it. (I'm a bit tempted to make the one
> already added to ecpg/Makefile conditional on the make version, as you
> suggest above, too.)
>
>

There is something odd about okapi, because my linux/gcc buildfarm
animal is using make 3.82 happily, with make_jobs = 4.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 11/28/2012 06:19 PM, Tom Lane wrote:
>> It appears to me that the case that okapi is hitting is specific to the
>> ecpg preprocessor build rules, and indeed specific to the case where
>> preproc.c needs to be rebuilt. A .NOTPARALLEL in ecpg/preproc/Makefile
>> would probably be enough to fix it. (I'm a bit tempted to make the one
>> already added to ecpg/Makefile conditional on the make version, as you
>> suggest above, too.)

> There is something odd about okapi, because my linux/gcc buildfarm
> animal is using make 3.82 happily, with make_jobs = 4.

Yeah, and nobody else has seen this either. It might just be that okapi
has exactly the right number of processors with exactly the right speeds
to make the failure a lot more probable. Or maybe there's something
weird about Gentoo's version of make (wouldn't be the first time).

Anyway, deparallelizing just the ecpg/preproc build would cost very
little in build time, since it's totally dominated by the preproc.c and
preproc.o build steps anyway. I'm inclined to just do it and see if
the problem goes away.

regards, tom lane

On Wed, 28 Nov 2012, Tom Lane wrote:

> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> > On 11/28/2012 02:14 PM, Alvaro Herrera wrote:
> >> Okapi has been failing sporadically on ecpg, and I wonder if it's
> >> related to this change.
>
> > Well, it looks like the make is broken and missing a clear dependency
> > requirement. I think we need to ask Jeremy to turn off parallel build
> > for okapi.
>
> Yeah, we already know that unpatched make 3.82 has got serious
> parallelism bugs:
> http://archives.postgresql.org/pgsql-hackers/2012-09/msg00397.php
>
> I wonder whether adding another .NOTPARALLEL directive would be a better
> idea than insisting people get hold of patched versions.

While we're talking about odd issues that only seem to happen on Okapi,
does anyone know of anything I can do to diagnose the pg_upgrade failure
on the 9.2 branch? There are no rogue (non-buildfarm-related)
postmaster/postgres processes running on the machine.

Jeremy Drake <pgbuildfarm(at)jdrake(dot)com> writes:
> While we're talking about odd issues that only seem to happen on Okapi,
> does anyone know of anything I can do to diagnose the pg_upgrade failure
> on the 9.2 branch? There are no rogue (non-buildfarm-related)
> postmaster/postgres processes running on the machine.

[ digs around ... ] It looks like the failure is coming from here:

if (strlen(path) >= sizeof(unp->sun_path))
return EAI_FAIL;

What's the size of the sun_path member of struct sockaddr_un on your
machine? I count 115 characters in your socket path ... maybe you
just need a less deeply nested test directory.

(If that is the problem, seems like we need to return something
more helpful than EAI_FAIL here.)

regards, tom lane

On Wed, 28 Nov 2012, Tom Lane wrote:

> Jeremy Drake <pgbuildfarm(at)jdrake(dot)com> writes:
> > While we're talking about odd issues that only seem to happen on Okapi,
> > does anyone know of anything I can do to diagnose the pg_upgrade failure
> > on the 9.2 branch? There are no rogue (non-buildfarm-related)
> > postmaster/postgres processes running on the machine.
>
> [ digs around ... ] It looks like the failure is coming from here:
>
> if (strlen(path) >= sizeof(unp->sun_path))
> return EAI_FAIL;
>
> What's the size of the sun_path member of struct sockaddr_un on your
> machine? I count 115 characters in your socket path ... maybe you
> just need a less deeply nested test directory.
>
> (If that is the problem, seems like we need to return something
> more helpful than EAI_FAIL here.)

/usr/include/sys/un.h: char sun_path[108]; /* Path name. */

That seems to be it. This may be just the excuse I needed to set up
dedicated users for my buildfarm animals.

On 11/28/2012 11:03 PM, Tom Lane wrote:
> Jeremy Drake <pgbuildfarm(at)jdrake(dot)com> writes:
>> While we're talking about odd issues that only seem to happen on Okapi,
>> does anyone know of anything I can do to diagnose the pg_upgrade failure
>> on the 9.2 branch? There are no rogue (non-buildfarm-related)
>> postmaster/postgres processes running on the machine.
> [ digs around ... ] It looks like the failure is coming from here:
>
> if (strlen(path) >= sizeof(unp->sun_path))
> return EAI_FAIL;
>
> What's the size of the sun_path member of struct sockaddr_un on your
> machine? I count 115 characters in your socket path ... maybe you
> just need a less deeply nested test directory.
>
> (If that is the problem, seems like we need to return something
> more helpful than EAI_FAIL here.)
>
>

Looks like it was. Good catch. What's the best way to fix?

Note that this problem was triggered by having a buildfarm buildroot
path of length about 49 or 50. I'm lucky not to have triggered it
myself. Do I need to add a check to limit the buildroot path length?

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 11/28/2012 11:03 PM, Tom Lane wrote:
>> [ digs around ... ] It looks like the failure is coming from here:
>>
>> if (strlen(path) >= sizeof(unp->sun_path))
>> return EAI_FAIL;

> Looks like it was. Good catch. What's the best way to fix?

So far as I can see, none of the spec-defined EAI_XXX codes map very
nicely to "path name too long". Possibly we could return EAI_SYSTEM
and set errno to ENAMETOOLONG, but I'm not sure the latter is very
portable either.

Another line of attack is to just teach getnameinfo_unix() to malloc its
result struct big enough to hold whatever the supplied path is. The
portability risk here is if sun_path is not the last field in struct
sockaddr_un on some platform --- but that seems a bit unlikely, and even
if it isn't I doubt we access any other members besides sun_family and
sun_path. I kind of like this approach, since it gets rid of the
length limitation rather than just reporting it more clearly.

regards, tom lane

I wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> Looks like it was. Good catch. What's the best way to fix?

> So far as I can see, none of the spec-defined EAI_XXX codes map very
> nicely to "path name too long". Possibly we could return EAI_SYSTEM
> and set errno to ENAMETOOLONG, but I'm not sure the latter is very
> portable either.

I tried this out and found that at least on Linux, gai_strerror() is too
stupid to pay attention to errno anyway; you just get "System error",
which is about as unhelpful as it could possibly be. I don't see any
way that we can get a more specific error message to be printed without
eliminating use of gai_strerror and providing our own infrastructure for
reporting getaddrinfo errors. While that wouldn't be incredibly awful
(we have such infrastructure already for ancient platforms...), it
still kinda sucks.

> Another line of attack is to just teach getaddrinfo_unix() to malloc its
> result struct big enough to hold whatever the supplied path is.

I tried this out too, and found that it doesn't work well, because both
libpq and the backend expect to be able to copy getaddrinfo results into
fixed-size SockAddr structs. We could probably fix that by adding
another layer of pointers and malloc operations, but it would be
somewhat invasive. Given the lack of prior complaints it's not clear
to me that it's worth that much trouble --- although getting rid of our
hard-wired assumptions about the maximum result size from getaddrinfo is
attractive from a robustness standpoint.

I'm a bit tempted to just replace EAI_FAIL with EAI_MEMORY here, so
that you'd get messages similar to "Memory allocation failure". That
might at least point your thoughts in the right direction, whereas
"Non-recoverable failure in name resolution" certainly conveys nothing
of use.

Thoughts?

regards, tom lane

On 11/29/2012 03:33 PM, Tom Lane wrote:
> I wrote:
>> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>> Looks like it was. Good catch. What's the best way to fix?
>> So far as I can see, none of the spec-defined EAI_XXX codes map very
>> nicely to "path name too long". Possibly we could return EAI_SYSTEM
>> and set errno to ENAMETOOLONG, but I'm not sure the latter is very
>> portable either.
> I tried this out and found that at least on Linux, gai_strerror() is too
> stupid to pay attention to errno anyway; you just get "System error",
> which is about as unhelpful as it could possibly be. I don't see any
> way that we can get a more specific error message to be printed without
> eliminating use of gai_strerror and providing our own infrastructure for
> reporting getaddrinfo errors. While that wouldn't be incredibly awful
> (we have such infrastructure already for ancient platforms...), it
> still kinda sucks.
>
>> Another line of attack is to just teach getaddrinfo_unix() to malloc its
>> result struct big enough to hold whatever the supplied path is.
> I tried this out too, and found that it doesn't work well, because both
> libpq and the backend expect to be able to copy getaddrinfo results into
> fixed-size SockAddr structs. We could probably fix that by adding
> another layer of pointers and malloc operations, but it would be
> somewhat invasive. Given the lack of prior complaints it's not clear
> to me that it's worth that much trouble --- although getting rid of our
> hard-wired assumptions about the maximum result size from getaddrinfo is
> attractive from a robustness standpoint.
>
> I'm a bit tempted to just replace EAI_FAIL with EAI_MEMORY here, so
> that you'd get messages similar to "Memory allocation failure". That
> might at least point your thoughts in the right direction, whereas
> "Non-recoverable failure in name resolution" certainly conveys nothing
> of use.
>
> Thoughts?

I don't think it's worth a heroic effort. Meanwhile I'll add a check in
the upgrade test module(s) to check for overly long paths likely to give
problems.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 11/29/2012 03:33 PM, Tom Lane wrote:
>>> Another line of attack is to just teach getaddrinfo_unix() to malloc its
>>> result struct big enough to hold whatever the supplied path is.
>> I tried this out too, and found that it doesn't work well, because both
>> libpq and the backend expect to be able to copy getaddrinfo results into
>> fixed-size SockAddr structs. We could probably fix that by adding
>> another layer of pointers and malloc operations, but it would be
>> somewhat invasive. Given the lack of prior complaints it's not clear
>> to me that it's worth that much trouble --- although getting rid of our
>> hard-wired assumptions about the maximum result size from getaddrinfo is
>> attractive from a robustness standpoint.

> I don't think it's worth a heroic effort. Meanwhile I'll add a check in
> the upgrade test module(s) to check for overly long paths likely to give
> problems.

I'm thinking maybe we should try to fix this. What's bugging me is that
Jeremy's build path doesn't look all that unreasonably long. The scary
scenario that's in the back of my mind is that one day somebody decides
to rearrange the Red Hat build servers a bit and suddenly I can't build
Postgres there anymore, because the build directory is nested a bit too
deep. Murphy's law would of course dictate that I find this out only
when under the gun to get a security update packaged.

regards, tom lane

On Thu, Nov 29, 2012 at 03:33:59PM -0500, Tom Lane wrote:
> I wrote:
> > So far as I can see, none of the spec-defined EAI_XXX codes map very
> > nicely to "path name too long". Possibly we could return EAI_SYSTEM
> > and set errno to ENAMETOOLONG, but I'm not sure the latter is very
> > portable either.
>
> I tried this out and found that at least on Linux, gai_strerror() is too
> stupid to pay attention to errno anyway; you just get "System error",
> which is about as unhelpful as it could possibly be. I don't see any
> way that we can get a more specific error message to be printed without
> eliminating use of gai_strerror and providing our own infrastructure for
> reporting getaddrinfo errors. While that wouldn't be incredibly awful
> (we have such infrastructure already for ancient platforms...), it
> still kinda sucks.

RFC 2553 and successor standards do not call for gai_strerror() to look at
anything other than its argument, so your finding for Linux surprises me less
than its alternative. Adopt code like "rc == EAI_SYSTEM ? strerror(errno) :
gai_strerror(rc)" to report the error, and your proposal to use ENAMETOOLONG
sounds suitable.

> > Another line of attack is to just teach getaddrinfo_unix() to malloc its
> > result struct big enough to hold whatever the supplied path is.
>
> I tried this out too, and found that it doesn't work well, because both
> libpq and the backend expect to be able to copy getaddrinfo results into
> fixed-size SockAddr structs. We could probably fix that by adding
> another layer of pointers and malloc operations, but it would be
> somewhat invasive. Given the lack of prior complaints it's not clear
> to me that it's worth that much trouble --- although getting rid of our
> hard-wired assumptions about the maximum result size from getaddrinfo is
> attractive from a robustness standpoint.

Linux enforces a hard limit matching the static buffer in sockaddr_un. You'd
proceed a bit further and hit "could not bind Unix socket: Invalid argument"
or some such.

I agree we should perhaps fix pg_upgrade to work even when its CWD is not
usable as a socket path. It could create a temporary directory under /tmp and
place the socket there, for example.

Thanks,
nm

Noah Misch <noah(at)leadboat(dot)com> writes:
> Linux enforces a hard limit matching the static buffer in sockaddr_un. You'd
> proceed a bit further and hit "could not bind Unix socket: Invalid argument"
> or some such.

Hm, I was wondering about that. The Single Unix Spec suggests that
bind/connect ought to accept pathnames at least up to PATH_MAX, but
if popular implementations don't honor that then it is a bit pointless
for us to do a lot of pushups in userspace.

> I agree we should perhaps fix pg_upgrade to work even when its CWD is not
> usable as a socket path. It could create a temporary directory under /tmp and
> place the socket there, for example.

Yeah, I was starting to think that pg_upgrade's test script is the real
culprit here. Every other variant of "make check" just puts the socket
in the default place, typically /tmp, so it's rather useless that this
one place is doing things differently.

Another thing that we should possibly consider if we're going to hack on
that is that "make check" is not currently very friendly to people who
try to move the default socket location to someplace other than /tmp,
such as the ever-popular /var/run/postgresql. The reason that this is
problematic is that /var/run/postgresql may not be there at all in a
build environment, and if it is, it's likely not writable by the user
you're running your build as. So just using the default socket
directory isn't real friendly in any case. In converting the Fedora
packages to use /var/run/postgresql recently, I found I had to add the
attached crude hacks to support running the regression tests during
build. It'd be nice if the consideration were handled by unmodified
sources ...

regards, tom lane

diff -Naur postgresql-9.2.0.sockets/contrib/pg_upgrade/test.sh postgresql-9.2.0/contrib/pg_upgrade/test.sh
--- postgresql-9.2.0.sockets/contrib/pg_upgrade/test.sh 2012-09-06 17:26:17.000000000 -0400
+++ postgresql-9.2.0/contrib/pg_upgrade/test.sh 2012-09-06 18:13:18.178092176 -0400
@@ -62,10 +62,14 @@
rm -rf "$logdir"
mkdir "$logdir"

+# we want the Unix sockets in $temp_root
+PGHOST=$temp_root
+export PGHOST
+
set -x

$oldbindir/initdb
-$oldbindir/pg_ctl start -l "$logdir/postmaster1.log" -w
+$oldbindir/pg_ctl start -l "$logdir/postmaster1.log" -o "-c unix_socket_directories='$PGHOST'" -w
if "$MAKE" -C "$oldsrc" installcheck; then
pg_dumpall -f "$temp_root"/dump1.sql || pg_dumpall1_status=$?
if [ "$newsrc" != "$oldsrc" ]; then
@@ -108,7 +112,7 @@

pg_upgrade -d "${PGDATA}.old" -D "${PGDATA}" -b "$oldbindir" -B "$bindir"

-pg_ctl start -l "$logdir/postmaster2.log" -w
+pg_ctl start -l "$logdir/postmaster2.log" -o "-c unix_socket_directories='$PGHOST'" -w

if [ $testhost = Msys ] ; then
cmd /c analyze_new_cluster.bat
diff -Naur postgresql-9.2.0.sockets/src/test/regress/pg_regress.c postgresql-9.2.0/src/test/regress/pg_regress.c
--- postgresql-9.2.0.sockets/src/test/regress/pg_regress.c 2012-09-06 17:26:17.000000000 -0400
+++ postgresql-9.2.0/src/test/regress/pg_regress.c 2012-09-06 18:13:18.184092537 -0400
@@ -772,7 +772,7 @@
if (hostname != NULL)
doputenv("PGHOST", hostname);
else
- unsetenv("PGHOST");
+ doputenv("PGHOST", "/tmp");
unsetenv("PGHOSTADDR");
if (port != -1)
{
@@ -2233,7 +2233,7 @@
*/
header(_("starting postmaster"));
snprintf(buf, sizeof(buf),
- SYSTEMQUOTE "\"%s/postgres\" -D \"%s/data\" -F%s -c \"listen_addresses=%s\" > \"%s/log/postmaster.log\" 2>&1" SYSTEMQUOTE,
+ SYSTEMQUOTE "\"%s/postgres\" -D \"%s/data\" -F%s -c \"listen_addresses=%s\" -c \"unix_socket_directories=/tmp\" > \"%s/log/postmaster.log\" 2>&1" SYSTEMQUOTE,
bindir, temp_install,
debug ? " -d 5" : "",
hostname ? hostname : "",

On 11/29/2012 05:20 PM, Tom Lane wrote:
>> I don't think it's worth a heroic effort. Meanwhile I'll add a check in
>> the upgrade test module(s) to check for overly long paths likely to give
>> problems.
> I'm thinking maybe we should try to fix this. What's bugging me is that
> Jeremy's build path doesn't look all that unreasonably long. The scary
> scenario that's in the back of my mind is that one day somebody decides
> to rearrange the Red Hat build servers a bit and suddenly I can't build
> Postgres there anymore, because the build directory is nested a bit too
> deep. Murphy's law would of course dictate that I find this out only
> when under the gun to get a security update packaged.
>
>

The only thing it breaks AFAIK is pg_upgrade testing because pg_upgrade
insists on setting the current directory as the socket dir. Maybe we
need a pg_upgrade option to specify the socket dir to use. Or maybe the
postmaster needs to check the length somehow before it calls
StreamServerPort() so we can give a saner error message.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> ... Or maybe the
> postmaster needs to check the length somehow before it calls
> StreamServerPort() so we can give a saner error message.

Hm. That's ugly, but a lot less invasive than trying to get the
official API to pass the information through. Sounds like a plan to me.

However, that's only addressing the reporting end of it; I think we
also need to change the pg_upgrade test script as suggested by Noah.

regards, tom lane

On 11/29/2012 06:23 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> ... Or maybe the
>> postmaster needs to check the length somehow before it calls
>> StreamServerPort() so we can give a saner error message.
> Hm. That's ugly, but a lot less invasive than trying to get the
> official API to pass the information through. Sounds like a plan to me.
>
> However, that's only addressing the reporting end of it; I think we
> also need to change the pg_upgrade test script as suggested by Noah.
>
>

The test script doesn't do anything. It's pg_upgrade itself that sets
the socket dir. See option.c:get_sock_dir()

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 11/29/2012 06:23 PM, Tom Lane wrote:
>> However, that's only addressing the reporting end of it; I think we
>> also need to change the pg_upgrade test script as suggested by Noah.

> The test script doesn't do anything. It's pg_upgrade itself that sets
> the socket dir. See option.c:get_sock_dir()

Um ... that's *another* place that needs to change then, because the
test script fires up postmasters on its own, outside of pg_upgrade.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> ... Or maybe the
>> postmaster needs to check the length somehow before it calls
>> StreamServerPort() so we can give a saner error message.

> Hm. That's ugly, but a lot less invasive than trying to get the
> official API to pass the information through. Sounds like a plan to me.

Here's a patch for that --- I think we should apply and back-patch this.

regards, tom lane

On 11/29/2012 07:16 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 11/29/2012 06:23 PM, Tom Lane wrote:
>>> However, that's only addressing the reporting end of it; I think we
>>> also need to change the pg_upgrade test script as suggested by Noah.
>> The test script doesn't do anything. It's pg_upgrade itself that sets
>> the socket dir. See option.c:get_sock_dir()
> Um ... that's *another* place that needs to change then, because the
> test script fires up postmasters on its own, outside of pg_upgrade.
>
>

True, but it doesn't do anything about setting the socket dir, so those
just get the compiled-in defaults. What exactly do you want to change
about the test script?

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 11/29/2012 07:16 PM, Tom Lane wrote:
>> Um ... that's *another* place that needs to change then, because the
>> test script fires up postmasters on its own, outside of pg_upgrade.

> True, but it doesn't do anything about setting the socket dir, so those
> just get the compiled-in defaults. What exactly do you want to change
> about the test script?

Well, I was thinking about also solving the problem that the compiled-in
default might be no good in a build environment.

regards, tom lane