Thread: pg_upgrade: Error out on too many command-line arguments
I propose the attached patch to make pg_upgrade error out on too many command-line arguments. This makes it match the behavior of other PostgreSQL programs. See [0] for an issue related to the lack of this check: [0]: https://www.postgresql.org/message-id/871sdbzizp.fsf%40jsievers.enova.com -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Sun, Aug 25, 2019 at 10:52 AM Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > I propose the attached patch to make pg_upgrade error out on too many > command-line arguments. This makes it match the behavior of other > PostgreSQL programs. > > See [0] for an issue related to the lack of this check: +1
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> I propose the attached patch to make pg_upgrade error out on too many
> command-line arguments. This makes it match the behavior of other
> PostgreSQL programs.
+1 ... are we missing this anywhere else?
regards, tom lane
On Sun, Aug 25, 2019 at 4:30 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > > I propose the attached patch to make pg_upgrade error out on too many > > command-line arguments. This makes it match the behavior of other > > PostgreSQL programs. > > +1 ... are we missing this anywhere else? I did some searching, and oid2name.c is also missing this.
On Sun, Aug 25, 2019 at 8:39 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
On Sun, Aug 25, 2019 at 4:30 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> > I propose the attached patch to make pg_upgrade error out on too many
> > command-line arguments. This makes it match the behavior of other
> > PostgreSQL programs.
>
> +1 ... are we missing this anywhere else?
I did some searching, and oid2name.c is also missing this.
Yes, "oid2name" missing that check too.
Ibrar Ahmed
Attachment
On Sun, Aug 25, 2019 at 05:10:47PM +0200, Julien Rouhaud wrote: > I did some searching, and oid2name.c is also missing this. And pgbench, no? -- Michael
Attachment
On Mon, Aug 26, 2019 at 9:46 AM Michael Paquier <michael@paquier.xyz> wrote:
On Sun, Aug 25, 2019 at 05:10:47PM +0200, Julien Rouhaud wrote:
> I did some searching, and oid2name.c is also missing this.
And pgbench, no?
Yes, the patch is slightly different.
--
Michael
Ibrar Ahmed
Attachment
On 2019-08-26 17:45, Ibrar Ahmed wrote: > On Mon, Aug 26, 2019 at 9:46 AM Michael Paquier <michael@paquier.xyz > <mailto:michael@paquier.xyz>> wrote: > > On Sun, Aug 25, 2019 at 05:10:47PM +0200, Julien Rouhaud wrote: > > I did some searching, and oid2name.c is also missing this. > > And pgbench, no? > > > Yes, the patch is slightly different. Thanks, pushed all that together. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello Peter, >> On Sun, Aug 25, 2019 at 05:10:47PM +0200, Julien Rouhaud wrote: >> > I did some searching, and oid2name.c is also missing this. >> >> And pgbench, no? >> >> Yes, the patch is slightly different. > > Thanks, pushed all that together. Great! Could we maintain coverage by adding a TAP test? See 1 liner attached. -- Fabien.
Attachment
On Fri, Aug 30, 2019 at 08:44:28AM +0200, Fabien COELHO wrote: > Could we maintain coverage by adding a TAP test? See 1 liner attached. I don't see why not. Perhaps this could be done for pgbench and oid2name as well? -- Michael
Attachment
Bonjour Michaël, > I don't see why not. Perhaps this could be done for pgbench and > oid2name as well? This is for pgbench. I did not found a TAP test in pg_upgrade, I do not think that it is worth creating one for that purpose. The "test.sh" script does not seem appropriate for this kind of coverage error test. The TAP test for oid2name only includes basic checks, options and arguments are not even tested, and there is no infra for actual testing, eg starting a server… Improving that would be a much more significant effort that the one line I added to pgbench existing TAP test. -- Fabien.
Fabien COELHO <coelho@cri.ensmp.fr> writes:
> Could we maintain coverage by adding a TAP test? See 1 liner attached.
Is this issue *really* worth expending test cycles on forevermore?
Test cycles are not free, and I see zero reason to think that a
check of this sort would ever catch any bugs. Now, if you had a
way to detect that somebody had forgotten the case in some new
program, that would be interesting.
regards, tom lane
Hello Tom, >> Could we maintain coverage by adding a TAP test? See 1 liner attached. > > Is this issue *really* worth expending test cycles on forevermore? With this argument consistently applied, postgres code coverage is consistently weak, with 25% of the code never executed, and 15% of functions never called. "psql" is abysmal, "libpq" is really weak. > Test cycles are not free, and I see zero reason to think that a > check of this sort would ever catch any bugs. Now, if you had a > way to detect that somebody had forgotten the case in some new > program, that would be interesting. It could get broken somehow, and the test would catch it? That would be the only command which tests this feature? This is a TAP test, not a test run on basic "make check". The cost is not measurable: pgbench 533 TAP tests run in 5 wallclock seconds, and this added test does not change that much. Now, if you say you are against it, then it is rejected… -- Fabien.
Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> Is this issue *really* worth expending test cycles on forevermore?
> With this argument consistently applied, postgres code coverage is
> consistently weak, with 25% of the code never executed, and 15% of
> functions never called. "psql" is abysmal, "libpq" is really weak.
It's all a question of balance. If you go too far in the other
direction, you end up with test suites that take an hour and a half
to run so nobody ever runs them (case in point, mysql). I'm all for
improving coverage in meaningful ways --- but cases like this seem
unlikely to be worth ongoing expenditures of testing effort.
regards, tom lane
>>> Is this issue *really* worth expending test cycles on forevermore? > >> With this argument consistently applied, postgres code coverage is >> consistently weak, with 25% of the code never executed, and 15% of >> functions never called. "psql" is abysmal, "libpq" is really weak. > > It's all a question of balance. If you go too far in the other > direction, you end up with test suites that take an hour and a half > to run so nobody ever runs them (case in point, mysql). I'm all for > improving coverage in meaningful ways --- but cases like this seem > unlikely to be worth ongoing expenditures of testing effort. Sure. I think there is room for several classes of tests, important ones always run and others run say by the farm, and I thought that is what TAP tests were for, given they are quite expensive anyway (eg most TAP test create their own postgres instance). So for me the suggestion was appropriate for a pgbench-specific TAP test. -- Fabien.