Re: Getting rid of regression test input/ and output/ files

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Getting rid of regression test input/ and output/ files
Date: 2021-12-19 21:08:07
Message-ID: 2207463.1639948087@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> This led me to wonder why we couldn't get rid of that entire
> mechanism in favor of some less-painful way of getting that
> information into the scripts. If we had the desired values in
> psql variables, we could do what we need easily, for example ...

Here's some fleshed-out patches for this.

0001 adds the \getenv command to psql; now with documentation
and a simple regression test.

0002 tweaks pg_regress to export the needed values as environment
variables, and modifies the test scripts to use those variables.
(For ease of review, this patch modifies the scripts in-place,
and then 0003 will move them.) A few comments on this:

* I didn't see any value in exporting @testtablespace@ as a separate
variable; we might as well let the test script know how to construct
that path name.

* I concluded that the right way to handle the concatenation issue
is *not* to rely on SQL literal concatenation, but to use psql's
\set command to concatenate parts of a string. In particular this
gives us a clean way to handle quoting/escaping rules in the places
where a pathname has to be embedded in some larger string, such as
a function body. The golden rule for that seems to be "use one \set
per level of quoting". I believe this code is now fairly proof
against situations that would completely break the existing way of
doing things, such as pathnames with quotes or backslashes in them.
(It's hard to test the embedded-quote case, because that breaks the
Makefiles too, but I did get through the regression tests with a
path including a backslash.)

* There are a couple of places where the existing tests involve
substituting a path name into expected query output or error messages.
This technique cannot handle that, but we have plenty of prior art for
dealing with such cases. I changed file_fdw to use a filter function
to hide the pathnames in EXPLAIN output, and tweaked create_function_0
to show only an edited version of an error message (this is based on a
similar case in infinite_recurse.sql).

0003 simply "git mv"'s the scripts and output files into place as
normal not-requiring-editing files. Be careful to "make clean"
before applying this, else you may have conflicts with the target
files already being present. Also, while you can run the tests
between 0003 and 0004, don't do "make clean" in this state or the
hacky EXTRA_CLEAN rules in dblink and file_fdw will remove files
you want.

0004 finally removes the no-longer-needed infrastructure in
pg_regress and the makefiles. (BTW, as far as I can find, the
MSVC scripts have no provisions for cleaning these generated files?)

There's some refactoring that could be done afterwards, for example
there seems little reason for dblink's paths.sql to continue to exist
as a separate script. But it seemed best for this patch series to
convert the scripts as mechanically as possible.

I'm fairly pleased with how this came out. I think these scripts
will be *much* easier to maintain in this form. Updating the
output/*.source files was always a major pain in the rear, since
you couldn't just copy results/ files to them.

Comments?

regards, tom lane

Attachment Content-Type Size
v1-0001-add-getenv-command.patch text/x-diff 4.6 KB
v1-0002-use-env-vars-in-tests.patch text/x-diff 61.5 KB
v1-0003-move-scripts-to-normal-dirs.patch text/x-diff 5.0 KB
v1-0004-remove-useless-infrastructure.patch text/x-diff 11.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2021-12-19 21:17:16 Re: sqlsmith: ERROR: XX000: bogus varno: 2
Previous Message Justin Pryzby 2021-12-19 20:54:22 sqlsmith: ERROR: XX000: bogus varno: 2