Thread: pgsql: Rework option set of vacuumlo
Rework option set of vacuumlo Like oid2name, vacuumlo has been lacking consistency with other utilities for its options: - Connection options gain long aliases. - Document environment variables which could be used: PGHOST, PGPORT and PGUSER. Documentation and code is reordered to be more consistent. A basic set of TAP tests has been added while on it. Author: Tatsuro Yamada Reviewed-by: Michael Paquier Discussion: https://postgr.es/m/c7e7f25c-1747-cd0f-9335-390bc97b2db5@lab.ntt.co.jp Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/bfea331a5e1b993d22071fc1696e6e8811d2d0d4 Modified Files -------------- contrib/vacuumlo/.gitignore | 2 ++ contrib/vacuumlo/Makefile | 6 ++++ contrib/vacuumlo/t/001_basic.pl | 9 +++++ contrib/vacuumlo/vacuumlo.c | 79 ++++++++++++++++++++++++----------------- doc/src/sgml/vacuumlo.sgml | 39 +++++++++++++++++--- 5 files changed, 98 insertions(+), 37 deletions(-)
Michael Paquier <michael@paquier.xyz> writes:
> Documentation and code is reordered to be more consistent. A basic set
> of TAP tests has been added while on it.
It'd be even better if these tests passed. For me, not so much:
$ make check
...
t/001_basic.pl .. # Looks like your test exited with 2 before it could output anything.
t/001_basic.pl .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 8/8 subtests
Test Summary Report
-------------------
t/001_basic.pl (Wstat: 512 Tests: 0 Failed: 0)
Non-zero exit status: 2
Parse errors: Bad plan. You planned 8 tests but ran 0.
Files=1, Tests=0, 0 wallclock secs ( 0.02 usr 0.01 sys + 0.06 cusr 0.01 csys = 0.10 CPU)
Result: FAIL
and the log file says
# Running: oid2name --help
Command 'oid2name' not found in /home/postgres/testversion/bin, /home/postgres/t
estversion/bin, /usr/local/autoconf-2.69/bin, /home/postgres/bin, /usr/lib64/qt-
3.3/bin, /usr/lib64/ccache, /usr/local/bin, /bin, /usr/bin, /usr/local/sbin, /us
r/sbin, /sbin at /home/postgres/pgsql/contrib/oid2name/../../src/test/perl/TestL
ib.pm line 414
# Looks like your test exited with 2 before it could output anything.
Similarly for the new vacuumlo tests.
This is also why the cfbot is unhappy. Oddly, the buildfarm is not.
I think the reason why not is that the buildfarm does "make installcheck"
in contrib, and it first does "make install" in contrib, so that there's a
copy of oid2name to be found in the PATH. But "make check" ought to be
testing the executable in the temporary installation tree, and it's not.
Digging, this is because the "make check" processing isn't actually
installing oid2name (resp. vacuumlo) into the temp installation tree :-(.
I don't think this is the fault of your tests, exactly --- there's
something wrong in the existing make rules. I'm not sure what though.
EXTRA_INSTALL should be getting set, according to pgxs.mk:
temp-install: EXTRA_INSTALL+=$(subdir)
but it evidently isn't.
regards, tom lane
I wrote:
> Digging, this is because the "make check" processing isn't actually
> installing oid2name (resp. vacuumlo) into the temp installation tree :-(.
>
> I don't think this is the fault of your tests, exactly --- there's
> something wrong in the existing make rules. I'm not sure what though.
> EXTRA_INSTALL should be getting set, according to pgxs.mk:
> temp-install: EXTRA_INSTALL+=$(subdir)
> but it evidently isn't.
Oh! The reason why not is that that line is inside "ifdef REGRESS",
and these subdirectories have no REGRESS tests. So we should move
that out to where it will apply regardless of that. Will fix.
regards, tom lane
On Tue, Aug 28, 2018 at 05:08:25PM -0400, Tom Lane wrote: > I wrote: >> Digging, this is because the "make check" processing isn't actually >> installing oid2name (resp. vacuumlo) into the temp installation tree :-(. >> >> I don't think this is the fault of your tests, exactly --- there's >> something wrong in the existing make rules. I'm not sure what though. >> EXTRA_INSTALL should be getting set, according to pgxs.mk: >> temp-install: EXTRA_INSTALL+=$(subdir) >> but it evidently isn't. > > Oh! The reason why not is that that line is inside "ifdef REGRESS", > and these subdirectories have no REGRESS tests. So we should move > that out to where it will apply regardless of that. Will fix. Yes, I was going to send a patch along the same lines as what you have just committed. Thanks! It looks that the binary was installed in my own PATH, so I did not notice it. This should definitely get back-patched in my opinion, I have similar problems with pg_rewind tests of 9.3 and 9.4 that I still maintain out of the core tree, where the main binary needs to be installed first. In this case I just used some Makefile trick. -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Aug 28, 2018 at 05:08:25PM -0400, Tom Lane wrote:
>> Oh! The reason why not is that that line is inside "ifdef REGRESS",
>> and these subdirectories have no REGRESS tests. So we should move
>> that out to where it will apply regardless of that. Will fix.
> Yes, I was going to send a patch along the same lines as what you have
> just committed. Thanks! It looks that the binary was installed in my
> own PATH, so I did not notice it.
> This should definitely get back-patched in my opinion, I have similar
> problems with pg_rewind tests of 9.3 and 9.4 that I still maintain out
> of the core tree, where the main binary needs to be installed first. In
> this case I just used some Makefile trick.
I didn't want to backpatch further than v11 without a test case that would
work in those branches, and I lacked one. If you've got out-of-core code
you could verify it with, please do that and back-patch further.
regards, tom lane
On Tue, Aug 28, 2018 at 05:47:47PM -0400, Tom Lane wrote: > I didn't want to backpatch further than v11 without a test case that would > work in those branches, and I lacked one. If you've got out-of-core code > you could verify it with, please do that and back-patch further. Was there any need to patch v11 with that actually? As there is nothing which needs except HEAD that does not seem strictly necessary. I have reviewed the modules I have, and actually it seems that I would not need much of that for a back-patch. One reason being that most of my TAP tests need pg_regress so as nodes can be initialized so this needs an external installation anyway. Maybe others have more thoughts to offer and would prefer a back-patch. -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Aug 28, 2018 at 05:47:47PM -0400, Tom Lane wrote:
>> I didn't want to backpatch further than v11 without a test case that would
>> work in those branches, and I lacked one. If you've got out-of-core code
>> you could verify it with, please do that and back-patch further.
> Was there any need to patch v11 with that actually?
I figured that patching v11 was safe without further testing, because
HEAD has hardly diverged from that. Previous branches probably should
get tested in some fashion before back-patching, and I didn't have a
good test case, so I didn't. But I suspect that a back-patch would
be worthwhile because (a) external modules would like to rely on such
infrastructure and/or (b) we might like to back-patch test cases for
contrib modules. However ...
> I have reviewed the modules I have, and actually it seems that I would
> not need much of that for a back-patch. One reason being that most of
> my TAP tests need pg_regress so as nodes can be initialized so this
> needs an external installation anyway.
... if there's other missing pieces then neither (a) nor (b) is very
compelling.
regards, tom lane