[PATCH] pg_regress and non-default unix socket path
Hi,
Debian has been patching pg_regress for years because our default unix
socket directory is /var/run/postgresql, but that is not writable by
the build user at build time. This used to be a pretty ugly "make-
patch-make check-unpatch-make install" patch dance, but now it is a
pretty patch that makes pg_regress accept --host=/tmp.
The patch is already part of the .deb files on apt.postgresql.org and
passes all regression test suites there.
Please consider it for 9.3. (And maybe backpatch it all the way...)
Christoph
--
cb@df7cb.de | http://www.df7cb.de/
Attachments:
pg_regress_unix_sockets.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
new file mode 100644
index b632326..d4d4279
*** a/src/test/regress/pg_regress.c
--- b/src/test/regress/pg_regress.c
*************** initialize_environment(void)
*** 835,840 ****
--- 835,841 ----
{
const char *pghost;
const char *pgport;
+ char pgportstr[32];
/*
* When testing an existing install, we honor existing environment
*************** initialize_environment(void)
*** 865,878 ****
pghost = "localhost";
#endif
! if (pghost && pgport)
! printf(_("(using postmaster on %s, port %s)\n"), pghost, pgport);
! if (pghost && !pgport)
! printf(_("(using postmaster on %s, default port)\n"), pghost);
! if (!pghost && pgport)
! printf(_("(using postmaster on Unix socket, port %s)\n"), pgport);
! if (!pghost && !pgport)
! printf(_("(using postmaster on Unix socket, default port)\n"));
}
convert_sourcefiles();
--- 866,883 ----
pghost = "localhost";
#endif
! /* Precompute the "port xxx" part so we don't have 6 printf()s below */
! if (pgport)
! snprintf(pgportstr, sizeof(pgportstr), _("port %s"), pgport);
! else
! snprintf(pgportstr, sizeof(pgportstr), _("default port"));
!
! if (pghost && *pghost != '/')
! printf(_("(using postmaster on %s, %s)\n"), pghost, pgportstr);
! else if (pghost)
! printf(_("(using postmaster on Unix socket %s, %s)\n"), pghost, pgportstr);
! else
! printf(_("(using postmaster on Unix socket, %s)\n"), pgportstr);
}
convert_sourcefiles();
*************** regression_main(int argc, char *argv[],
*** 2249,2255 ****
*/
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,
bindir, temp_install,
debug ? " -d 5" : "",
hostname ? hostname : "",
--- 2254,2262 ----
*/
header(_("starting postmaster"));
snprintf(buf, sizeof(buf),
! hostname && *hostname == '/'
! ? SYSTEMQUOTE "\"%s/postgres\" -D \"%s/data\" -F%s -k \"%s\" > \"%s/log/postmaster.log\" 2>&1" SYSTEMQUOTE
! : SYSTEMQUOTE "\"%s/postgres\" -D \"%s/data\" -F%s -c \"listen_addresses=%s\" > \"%s/log/postmaster.log\" 2>&1" SYSTEMQUOTE,
bindir, temp_install,
debug ? " -d 5" : "",
hostname ? hostname : "",
Re: To PostgreSQL Hackers 2013-04-09 <20130409120807.GD26705@msgid.df7cb.de>
If the patch looks too intrusive at this stage of the release, it
would be enough if the last chunk got included, which should really be
painless:
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c new file mode 100644 index b632326..d4d4279 *** a/src/test/regress/pg_regress.c --- b/src/test/regress/pg_regress.c
[...]
*************** regression_main(int argc, char *argv[], *** 2249,2255 **** */ 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, bindir, temp_install, debug ? " -d 5" : "", hostname ? hostname : "", --- 2254,2262 ---- */ header(_("starting postmaster")); snprintf(buf, sizeof(buf), ! hostname && *hostname == '/' ! ? SYSTEMQUOTE "\"%s/postgres\" -D \"%s/data\" -F%s -k \"%s\" > \"%s/log/postmaster.log\" 2>&1" SYSTEMQUOTE ! : SYSTEMQUOTE "\"%s/postgres\" -D \"%s/data\" -F%s -c \"listen_addresses=%s\" > \"%s/log/postmaster.log\" 2>&1" SYSTEMQUOTE, bindir, temp_install, debug ? " -d 5" : "", hostname ? hostname : "",
Christoph
--
cb@df7cb.de | http://www.df7cb.de/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Apr 9, 2013 at 8:08 AM, Christoph Berg <cb@df7cb.de> wrote:
Debian has been patching pg_regress for years because our default unix
socket directory is /var/run/postgresql, but that is not writable by
the build user at build time. This used to be a pretty ugly "make-
patch-make check-unpatch-make install" patch dance, but now it is a
pretty patch that makes pg_regress accept --host=/tmp.The patch is already part of the .deb files on apt.postgresql.org and
passes all regression test suites there.Please consider it for 9.3. (And maybe backpatch it all the way...)
The hunk that changes the messages might need some thought so that it
doesn't cause a translation regression. But in general I see no
reason not to do this before we release beta1. It seems safe enough,
and changes that reduce the need for packagers to carry private
patches are, I think, generally a good thing.
--
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
Robert Haas escribió:
On Tue, Apr 9, 2013 at 8:08 AM, Christoph Berg <cb@df7cb.de> wrote:
Debian has been patching pg_regress for years because our default unix
socket directory is /var/run/postgresql, but that is not writable by
the build user at build time. This used to be a pretty ugly "make-
patch-make check-unpatch-make install" patch dance, but now it is a
pretty patch that makes pg_regress accept --host=/tmp.The patch is already part of the .deb files on apt.postgresql.org and
passes all regression test suites there.Please consider it for 9.3. (And maybe backpatch it all the way...)
The hunk that changes the messages might need some thought so that it
doesn't cause a translation regression.
FWIW I don't think we translate pg_regress at all, and I have my doubts
that it makes much sense. I'd go as far as suggest we get rid of the _()
decorations in the lines we're changing.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
Robert Haas <robertmhaas@gmail.com> writes:
The hunk that changes the messages might need some thought so that it
doesn't cause a translation regression. But in general I see no
reason not to do this before we release beta1. It seems safe enough,
and changes that reduce the need for packagers to carry private
patches are, I think, generally a good thing.
It looks to me like this is asking for pg_regress to adopt a nonstandard
interpretation of PGHOST, which doesn't seem like a wise thing at all,
especially if it's not documented.
FWIW, the equivalent thing in the Red Hat/Fedora packages can be seen
in this patch:
http://pkgs.fedoraproject.org/cgit/postgresql.git/plain/postgresql-var-run-socket.patch
which would not get noticeably shorter if we hacked pg_regress in the
suggested way. AFAICT, instead of touching pg_regress.c, Red Hat's
patch would need to do something to the regression Makefiles if we
wanted to use this implementation. I'm not convinced that'd be better
at all. TBH, if this is committed, the Red Hat patches will probably
end up reverting it.
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: Tom Lane 2013-04-12 <20318.1365786018@sss.pgh.pa.us>
Robert Haas <robertmhaas@gmail.com> writes:
The hunk that changes the messages might need some thought so that it
doesn't cause a translation regression. But in general I see no
reason not to do this before we release beta1. It seems safe enough,
and changes that reduce the need for packagers to carry private
patches are, I think, generally a good thing.It looks to me like this is asking for pg_regress to adopt a nonstandard
interpretation of PGHOST, which doesn't seem like a wise thing at all,
especially if it's not documented.
If you are saying pg_regress isn't always honoring PGHOST because it
(re-)sets the variable itself, that's not the fault of my patch. My
patch fixes pg_regress to make "--host /tmp" work, which is just the
same thing as "psql --host /tmp" does.
It's undocumented just like the existing behavior (--host works in
temp-install mode) is undocumented. I can fix that, my original goal
was to change as little as possible in the code.
The only thing that doesn't work anymore is that you cannot set both
listen_addresses *and* unix_socket_directories at the same time
anymore. Does Red Hat need that functionality? I don't think
regression tests need that flexibility so it's worth the trouble. (The
proper fix would probably be to get rid of --host in temp-install mode
and expose listen_addresses and unix_socket_director{y,ies} directly.)
FWIW, the equivalent thing in the Red Hat/Fedora packages can be seen
in this patch:http://pkgs.fedoraproject.org/cgit/postgresql.git/plain/postgresql-var-run-socket.patch
This is exactly the opposite of what my patch is doing. The Red Hat
patch hardcodes /tmp, which my patch is not. We use to have such a
patch, and it was bad, because we needed to apply and revert the patch
at built time, such that the version in the .deb didn't have the
hardcoded path.
For reference, here's the old Debian patch (the 9.1 packages still
have it, the new one is in 9.2):
In lines 139 ff of debian/rules, the patch gets applied and reverted:
http://anonscm.debian.org/loggerhead/pkg-postgresql/postgresql-9.1/trunk/annotate/head:/debian/rules
which would not get noticeably shorter if we hacked pg_regress in the
suggested way. AFAICT, instead of touching pg_regress.c, Red Hat's
patch would need to do something to the regression Makefiles if we
wanted to use this implementation. I'm not convinced that'd be better
at all. TBH, if this is committed, the Red Hat patches will probably
end up reverting it.
You are already setting PGHOST in contrib/pg_upgrade/test.sh. This
would just need to be replaced by "EXTRA_REGRESS_OPTS='--host=/tmp'".
Then your pg_regress.c chunk could go away.
(To put it another way, you don't seem to be using pg_regress --host
here, you shouldn't be affected by this change.)
I'm open to suggestions of how to better fix this in pg_regress.
Though could we perhaps first apply this patch as a first step in the
right direction?
Christoph
--
cb@df7cb.de | http://www.df7cb.de/
On Fri, Apr 12, 2013 at 1:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
The hunk that changes the messages might need some thought so that it
doesn't cause a translation regression. But in general I see no
reason not to do this before we release beta1. It seems safe enough,
and changes that reduce the need for packagers to carry private
patches are, I think, generally a good thing.It looks to me like this is asking for pg_regress to adopt a nonstandard
interpretation of PGHOST, which doesn't seem like a wise thing at all,
especially if it's not documented.
I see it the other way around. Most places in PostgreSQL that allow a
hostname also allow a string beginning with a slash to be specified
instead, which then gets interpreted as a socket directory name.
pg_regress does not allow that, and this patch would fix that.
FWIW, the equivalent thing in the Red Hat/Fedora packages can be seen
in this patch:http://pkgs.fedoraproject.org/cgit/postgresql.git/plain/postgresql-var-run-socket.patch
which would not get noticeably shorter if we hacked pg_regress in the
suggested way. AFAICT, instead of touching pg_regress.c, Red Hat's
patch would need to do something to the regression Makefiles if we
wanted to use this implementation. I'm not convinced that'd be better
at all. TBH, if this is committed, the Red Hat patches will probably
end up reverting it.
The Red Hat patch is aiming to change the run-time behavior of the
server, which Christoph's patch is not. The net effect would be that
the last two hunks could be ditched in favor of setting
EXTRA_REGRESS_OPTS. I don't imagine that's a big improvement but it
doesn't seem like a step backward, either. I can certainly see the
appeal: IME, it's much nicer to pass in a few extra configuration
options than to have to patch the source.
--
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