Thread: pg_regress in C

Per discussion at the conference:

In order to run the regression tests on Windows without msys, pg_regress
needs to be reimplemnted in C. There are two reasons to want this:

1) To be able to build completely without mingw (which can be a pain to
install if you're not lucky) - build pg with visual c++, run tests
directly with pg_regress.

2) To be able to ship the regression tests with the installer, where we
really can't expect msys to exist. The reason for this is twofold in
turn - one is to simply allow a way to "prove to PHBs that the installed
version works", and realistically also to test that linked-in DLLs that
we don't ship somehow broke the system. It could happen...



This implementation only replaces the shellscript with a C program. It
still uses psql to execute the queries (thus testing psql as well), and
shells to "diff" to generate diffs. (having to ship diff on win32 is
still not perfect, but a *lot* better than msys/mingw). I have tried to
keep things as similar as possible for now - possible enhancements that
can be done in C but not in SH can be added at a later time.

I have so far test on win32 (running "make installcheck" equivalent with
a MSVC-compiled backend) and on Linux (running "make check" equivalent),
with no problems. I haven't modified the Makefile, so it need to be run
manually. The commandline is almost the same - needs an added --platform
set to the value that's available in the Makefile only. A "bindir"
parameter has been added to be able to run it on an installed system not
in the system PATH.

To use regexp, this code links with regcomp.c and regexec.c from
src/backend/regex, and with wstrncmp.c from src/backend/utils/mb. I have
not updated the Makefile yet, I don't know if we'd want to just link
with the .o files or copy in the .c ones (might be an issue for the
regex ones which include other .c files). I've been building it in a
completely separate sourcetree.


The code still needs some cleanup (for example making the error messages
more in line with our messaging guidlines), but I wanted to get this
submitted as soon as possible because of the upcoming feature freeze. In
a couple of days I will only have sporadic internet access for several
weeks, and thus won't be able to work on updates during that time. I'm
therefor hopeing to get this submitted and catch some early comments
now. Hopefully it will be enough to get it on the queue and then I can
fix whatever issues are when I get back to full connectivity. Unless,
that is, the whole approach seems wrong, in which case there's not much
point to me working on it anyway :-)

(I know, not the best of planning, but that's how feature-freeze-date
turned out for me :-( Sorry..)


//Magnus


Note: This version does not implement max_connections. From what I can
tell that's only used to workaround some bug in cygwin shells, which
shouldn't appear anymore after this is used. Not tested, have no working
cygwin env and haven't managed to install one..

"Magnus Hagander" <mha@sollentuna.net> writes:
> Per discussion at the conference:
> In order to run the regression tests on Windows without msys, pg_regress
> needs to be reimplemnted in C.

This has some minor portability issues (macros with ... aren't portable,
for instance) but I think it's something we need to do.  Barring
objections I'm going to clean up and apply it.

            regards, tom lane

On July 18, 1:06 am Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Magnus Hagander" <mha@sollentuna.net> writes:
> >  Per discussion at the conference:
> >  In order to run the regression tests on Windows without msys,
> >  pg_regress needs to be reimplemnted in C.
>
> This has some minor portability issues (macros with ... aren't portable,
> for instance) but I think it's something we need to do.  Barring
> objections I'm going to clean up and apply it.

I've sent Magnus a patch a few days ago (but he seems to be without
internet connection already), maybe it can help you. I just fixed it such
that it worked for my non-standard install path.


I also did a small patch for the Makefile to compile and call pg_regress.

I append both patches (and hence won't send this mail to -hackers)

I also proposed to make pg_regress more modular some time ago:

http://archives.postgresql.org/pgsql-patches/2006-06/msg00212.php

A similar approach should be kept in mind as an enhancement to the C version.


Joachim

"Magnus Hagander" <mha@sollentuna.net> writes:
> Per discussion at the conference:
> In order to run the regression tests on Windows without msys, pg_regress
> needs to be reimplemnted in C.

Patch committed after significant further work.  As committed,
pg_regress.c is pretty nearly an exact replacement for the shell script;
the only significant deviation is that the --temp_install switch's
argument is required not optional.  (This is because our homegrown
version of getopt_long doesn't allow optional arguments.  Maybe that
should be fixed sometime.)

There is one possibly important loose end: the shell script makes an
effort to catch signals and shut down the temp postmaster before
exiting, while there's no such code in the C version.  I'm not sure
if it's necessary.  At least on my machine, if you type control-C while
the tests are running then the kernel sends SIGINT to everything that's
part of the terminal's process group, which will include the postmaster
--- so the shutdown happens anyway.  I have no idea if that'll work on
Windows...  One reason I didn't try to do this is I'm a bit hesitant to
write a signal handler that does anything as interesting as a system()
call, which would seem to be necessary to duplicate what the shell
script did.  Comments?

            regards, tom lane

On Tue, Jul 18, 2006 at 10:46:04PM -0400, Tom Lane wrote:
> ...  One reason I didn't try to do this is I'm a bit hesitant to
> write a signal handler that does anything as interesting as a system()
> call, which would seem to be necessary to duplicate what the shell
> script did.  Comments?

It might not actually be unsafe, given system() actually blocks on
waitpid() which is specifically listed as a "safe" function. I'm a bit
confused though, because system() generally sets the parent to ignore
SIGINT which running the child process. That means the postmaster is
being killed but pg_regress is not? If this is the case, then you won't
be able to catch SIGINT anyway.

Also, the kernel sending it to everyone on the same terminal is (AIUI)
only true if you're running under the same session ID. postgres only
daemonizes itself to be immune from frontend terminal interrupts (using
setsid) when silent_mode is on. I think it defaults to off, which is
probably why it works at all.

Anyway, the signal handling for Windows involves a seperate thead AIUI
which may make it easier. It might be interesting to see how bash does
it.

Hope this helps,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Martijn van Oosterhout <kleptog@svana.org> writes:
> On Tue, Jul 18, 2006 at 10:46:04PM -0400, Tom Lane wrote:
>> ...  One reason I didn't try to do this is I'm a bit hesitant to
>> write a signal handler that does anything as interesting as a system()
>> call, which would seem to be necessary to duplicate what the shell
>> script did.  Comments?

> It might not actually be unsafe, given system() actually blocks on
> waitpid() which is specifically listed as a "safe" function. I'm a bit
> confused though, because system() generally sets the parent to ignore
> SIGINT which running the child process. That means the postmaster is
> being killed but pg_regress is not? If this is the case, then you won't
> be able to catch SIGINT anyway.

The cases of interest are where the (new) code goes through
spawn_process, which does a fork() and then calls system() in the
child.  So the intermediate child is probably SIGINT-blocked, but
pg_regress itself isn't.

I was planning to rewrite spawn_process anyway, because I noticed that
as it's currently set up, we are actually creating four(!) processes per
parallel test: the pg_regress child, the shell invoked by system, the
psql invoked by the shell, and the connected backend.  That's even worse
than the shell script, which (at least on my system) used three processes.
I believe we can get down to two (psql and backend) if spawn_process
exec's the shell instead of using system, and also puts "exec" in front
of the command string passed to the shell.  So in that scenario there'd
not be any signal-blocking going on anyway.

That still leaves us with the question of whether pg_regress needs to do
anything special when signaled, though.

            regards, tom lane

> > Per discussion at the conference:
> > In order to run the regression tests on Windows without msys,
> > pg_regress needs to be reimplemnted in C.
>
> This has some minor portability issues (macros with ... aren't
> portable, for instance) but I think it's something we need to do.
> Barring objections I'm going to clean up and apply it.

Thanks for this, including all the followup patches :-)


//Magnus