Re: pgbench tap tests & minor fixes

Hello Nikolay,

> Hi! Since I used to be good at perl, I like tests, and I've dealt with
> postgres TAP tests before, I think I can review you patch. If it is OK for
> everyone.

I think that all good wills are welcome, especially concerning code
reviews.

> For now I've just gave this patch a quick look through... But nevertheless I
> have something to comment:
>
>> The attached patch:
>>
>> (1) extends the existing perl tap test infrastructure with methods to test
>> pgbench, i.e. "pgbench" which runs a pgbench test and "pgbench_likes"
>> which allows to check for expectations.

> I do not think it is good idea adding this functions to the PostgresNode.pm.

I thought it was:-)

> They are pgbench specific.

Sure.

> I do not think anybody will need them outside of pgbench tests.

Hmmm. The pre-existing TAP test in pgbench is about concurrent commits, it
is not to test pgbench itself. Pgbench allows to run some programmable
clients in parallel very easily, which cannot be done simply otherwise. I
think that having it there would encourage such uses.

Another point is that the client needs informations held as attributes in
the node in order to connect to the server. Having it outside would mean
picking the attributes inside, which is pretty unclean as it breaks the
abstraction. For me, all pg standard executables could have their methods
in PostgresNode.pm.

Finally, it does not cost anything to have it there.

> Generally, these functions as they are now, should be placed is separate .pm
> file. like it was done in src/test/ssl/

I put them there because it already manages both terminal client (psql) &
server side things (initdb, postgres...). Initially pgbench was a
"contrib" but it has been moved as a standard client a few versions ago,
so for me it seemed logical to have the support there.

> May be you should move some code into some kind of helpers and keep them in
> PostgresNode.pm.

Hmmm. I can put a "run any client" function with the same effect and pass
an additional argument to tell that pgbench should be run, but this looks
quite contrived for no special reason.

> Or write universal functions that can be used for testing any postgres console
> tool. Then they can be placed in PostgresNode.pm

There are already "psql"-specific functions... Why not pgbench specific
ones?

>> (3) add a lot of new very small tests so that coverage jumps from very low
>> to over 90% for source files. [...]
>
> What tool did you use to calculate the coverage?

I followed the documentated recipee:

https://www.postgresql.org/docs/devel/static/regress-coverage.html

> Lots of small test looks good at first glance, except the point that adding
> lots of small files with pgbench scripts is not great idea. This makes code
> tree too overloaded with no practical reason. I am speaking of
> src/bin/pgbench/t/scripts/001_0* files.

> I think all the data from this file should be somehow imported into
> 001_pgbench.pl and these files should be created only when tests is running,
> and then removed when testing is done.

Hmmm. I thought of that but was very unconvinced by the approach: pgbench
basically always requires a file, so what the stuff was doing was writting
the script into a file, checking for possible errors when doing so, then
unlinking the file and checking for errors again after the run. Then you
have to do some escaping the the pgbench script themselves, and the perl
script becomes pretty horrible and unreadable with plenty of perl, SQL,
backslash commands in strings... Finally, if the script is inside the perl
script it makes it hard to run the test outside of it when a problem is
found, so it is a pain.

> I think that job for creating and removing these files should be moved to
> pgbench and pgbench_likes. But there is more then one way to do it. ;-)

Hmmm. See above.

> That's what I've noticed so far... I will give this patch more attentive look
> soon.

--
Fabien.