Thread: [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/
Attachment
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/
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
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
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
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):
http://anonscm.debian.org/loggerhead/pkg-postgresql/postgresql-9.1/trunk/annotate/head:/debian/pg_regress-in-tmp.patch
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