Re: [PATCH] pg_regress and non-default unix socket path

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/