Horton

Thread

Commits

  1. Add \warn command to psql.

  1. [PATCH v1] Add \echo_stderr to psql

    David Fetter <david@fetter.org> — 2019-04-21T18:31:15Z

    Folks,

Any interest in this?

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

  2. Re: [PATCH v1] Add \echo_stderr to psql

    Pavel Stehule <pavel.stehule@gmail.com> — 2019-04-21T18:42:18Z

    ne 21. 4. 2019 v 20:31 odesílatel David Fetter <david@fetter.org> napsal:

> Folks,
>
> Any interest in this?
>

has sense

Pavel


> Best,
> David.
> --
> David Fetter <david(at)fetter(dot)org> http://fetter.org/
> Phone: +1 415 235 3778
>
> Remember to vote!
> Consider donating to Postgres: http://www.postgresql.org/about/donate
>

  3. Re: [PATCH v1] Add \echo_stderr to psql

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-04-21T19:31:16Z

    > Any interest in this?

Yep, although I'm not sure of the suggested command name. More 
suggestions:
   \stderr ...
   \err ...
   \error ...
   \warn ...
   \warning ...

-- 
Fabien.

  4. Re: [PATCH v1] Add \echo_stderr to psql

    David Fetter <david@fetter.org> — 2019-04-21T21:38:20Z

    On Sun, Apr 21, 2019 at 09:31:16PM +0200, Fabien COELHO wrote:
> > Any interest in this?
> 
> Yep, although I'm not sure of the suggested command name. More suggestions:
>   \stderr ...
>   \err ...
>   \error ...
>   \warn ...
>   \warning ...

Naming Things is one of the two[1] hard problems in CS.

I'm happy with whatever the community consensus comes out to be.

Best,
David.

[1] The others are cache coherency and off-by-one
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

  5. Re: [PATCH v1] Add \echo_stderr to psql

    Corey Huinker <corey.huinker@gmail.com> — 2019-04-22T03:52:45Z

    >
>
>    \warn ...
>    \warning ...
>

These two seem about the best to me, drawing from the perl warn command.

I suppose we could go the bash &2 route here, but I don't want to.

  6. Re: [PATCH v1] Add \echo_stderr to psql

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-04-22T07:04:08Z

    Hello Corey,

>>    \warn ...
>>    \warning ...
>
> These two seem about the best to me, drawing from the perl warn command.

Yep, I was thinking of perl & gmake. Maybe the 4 letter option is better 
because its the same length as "echo".

> I suppose we could go the bash &2 route here, but I don't want to.

I agree on this one.

-- 
Fabien.

  7. Re: [PATCH v1] Add \echo_stderr to psql

    David Fetter <david@fetter.org> — 2019-04-22T13:45:32Z

    On Mon, Apr 22, 2019 at 09:04:08AM +0200, Fabien COELHO wrote:
> 
> Hello Corey,
> 
> > >    \warn ...
> > >    \warning ...
> > 
> > These two seem about the best to me, drawing from the perl warn command.
> 
> Yep, I was thinking of perl & gmake. Maybe the 4 letter option is better
> because its the same length as "echo".
> 
> > I suppose we could go the bash &2 route here, but I don't want to.
> 
> I agree on this one.

Please find attached v2, name is now \warn.

How might we test this portably?

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

  8. Re: [PATCH v1] Add \echo_stderr to psql

    Fabien COELHO <fabien.coelho@mines-paristech.fr> — 2019-04-22T17:46:18Z

    >>>>    \warn ...
>>>>    \warning ...
>>>
>>> These two seem about the best to me, drawing from the perl warn command.
>>
>> Yep, I was thinking of perl & gmake. Maybe the 4 letter option is better
>> because its the same length as "echo".
>>
>>> I suppose we could go the bash &2 route here, but I don't want to.
>>
>> I agree on this one.
>
> Please find attached v2, name is now \warn.
>
> How might we test this portably?

TAP testing? see pgbench which has tap test which can test stdout & stderr 
by calling utility command_checks_all, the same could be done with psql.

-- 
Fabien.

  9. Re: [PATCH v1] Add \echo_stderr to psql

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-04-27T14:05:20Z

    Hello David,

> Please find attached v2, name is now \warn.

Patch applies cleanly, compiles, "make check ok", although there are no 
tests. Doc gen ok.

Code is pretty straightforward.

I'd put the commands in alphabetical order (echo, qecho, warn) instead of 
e/w/q in the condition.

The -n trick does not appear in the help lines, ISTM that it could fit, so 
maybe it could be added, possibly something like:

  \echo [-n] [TEXT]  write string to stdout, possibly without trailing newline

and same for \warn and \qecho?

> How might we test this portably?

Hmmm... TAP tests are expected to be portable. Attached a simple POC, 
which could be extended to test many more things which are currently out 
of coverage (src/bin/psql stuff is covered around 40% only).

-- 
Fabien.

  10. Re: [PATCH v1] Add \echo_stderr to psql

    David Fetter <david@fetter.org> — 2019-04-27T17:15:48Z

    On Sat, Apr 27, 2019 at 04:05:20PM +0200, Fabien COELHO wrote:
> 
> Hello David,
> 
> > Please find attached v2, name is now \warn.
> 
> Patch applies cleanly, compiles, "make check ok", although there are no
> tests. Doc gen ok.
> 
> Code is pretty straightforward.
> 
> I'd put the commands in alphabetical order (echo, qecho, warn) instead of
> e/w/q in the condition.

Done.

> The -n trick does not appear in the help lines, ISTM that it could fit, so
> maybe it could be added, possibly something like:
> 
>  \echo [-n] [TEXT]  write string to stdout, possibly without trailing newline
> 
> and same for \warn and \qecho?

Makes sense, but I put it there just for \echo to keep lines short.

> > How might we test this portably?
> 
> Hmmm... TAP tests are expected to be portable. Attached a simple POC, which
> could be extended to test many more things which are currently out of
> coverage (src/bin/psql stuff is covered around 40% only).

Thanks for putting this together. I've added this test, and agree that
increasing coverage is important for another patch.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

  11. Re: [PATCH v1] Add \echo_stderr to psql

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-04-27T20:09:27Z

    Hello David,

About v3. Applies, compiles, global & local make check are ok. doc gen ok.

>> I'd put the commands in alphabetical order (echo, qecho, warn) instead of
>> e/w/q in the condition.
>
> Done.

Cannot see it:

   + else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "warn") == 0 || strcmp(cmd, "qecho") == 0)

>> The -n trick does not appear in the help lines, ISTM that it could fit, so
>> maybe it could be added, possibly something like:
>>
>>  \echo [-n] [TEXT]  write string to stdout, possibly without trailing newline
>>
>> and same for \warn and \qecho?
>
> Makes sense, but I put it there just for \echo to keep lines short.

I think that putting together the 3 echo variants help makes sense, but 
maybe someone will object about breaking the abc order.

>>> How might we test this portably?
>>
>> Hmmm... TAP tests are expected to be portable. Attached a simple POC, which
>> could be extended to test many more things which are currently out of
>> coverage (src/bin/psql stuff is covered around 40% only).
>
> Thanks for putting this together. I've added this test, and agree that
> increasing coverage is important for another patch.

Yep.

-- 
Fabien.

  12. [PATCH v4] Add \warn to psql

    David Fetter <david@fetter.org> — 2019-04-28T14:58:13Z

    On Sat, Apr 27, 2019 at 10:09:27PM +0200, Fabien COELHO wrote:
> 
> Hello David,
> 
> About v3. Applies, compiles, global & local make check are ok. doc gen ok.
> 
> > > I'd put the commands in alphabetical order (echo, qecho, warn) instead of
> > > e/w/q in the condition.
> > 
> > Done.
> 
> Cannot see it:
> 
>   + else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "warn") == 0 || strcmp(cmd, "qecho") == 0)

My mistake. I didn't think the order in which they were compared
mattered much, but it makes sense on further reflection to keep things
tidy in the code.

> > > The -n trick does not appear in the help lines, ISTM that it could fit, so
> > > maybe it could be added, possibly something like:
> > > 
> > >  \echo [-n] [TEXT]  write string to stdout, possibly without trailing newline
> > > 
> > > and same for \warn and \qecho?
> > 
> > Makes sense, but I put it there just for \echo to keep lines short.
> 
> I think that putting together the 3 echo variants help makes sense, but
> maybe someone will object about breaking the abc order.

Here's the alphabetical version.

> > > > How might we test this portably?
> > > 
> > > Hmmm... TAP tests are expected to be portable. Attached a simple POC, which
> > > could be extended to test many more things which are currently out of
> > > coverage (src/bin/psql stuff is covered around 40% only).
> > 
> > Thanks for putting this together. I've added this test, and agree that
> > increasing coverage is important for another patch.
> 
> Yep.

Speaking of which, I'd like to see about getting your patch against
Testlib.pm in so more tests of psql can also go in. It's not a new
feature /per se/, and it doesn't break any current scripts, so I'd
make the argument that it's OK for them to go in and possibly even be
back-patched.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

  13. Re: [PATCH v4] Add \warn to psql

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-04-28T18:22:09Z

    Hello David,

About v4: applies, compiles, global & local "make check" ok. Doc gen ok.

Code & help look ok.

About the doc: I do not understand why the small program listing contains 
an "\echo :variable". Also, the new entry should probably be between the 
\w & \watch entries instead of between \echo & \ef.

-- 
Fabien.

  14. Re: [PATCH v4] Add \warn to psql

    David Fetter <david@fetter.org> — 2019-04-29T04:11:06Z

    On Sun, Apr 28, 2019 at 08:22:09PM +0200, Fabien COELHO wrote:
> 
> Hello David,
> 
> About v4: applies, compiles, global & local "make check" ok. Doc gen ok.
> 
> Code & help look ok.
> 
> About the doc: I do not understand why the small program listing contains an
> "\echo :variable".

It no longer does.

> Also, the new entry should probably be between the \w &
> \watch entries instead of between \echo & \ef.

Moved.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

  15. Re: [PATCH v4] Add \warn to psql

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-04-29T06:30:18Z

    Hello David,

About v5: applies, compiles, global & local make check ok, doc gen ok.

Very minor comment: \qecho is just before \o in the embedded help, where 
it should be just after. Sorry I did not see it on the preceding 
submission.

-- 
Fabien.

  16. Re: [PATCH v4] Add \warn to psql

    David Fetter <david@fetter.org> — 2019-04-29T14:17:10Z

    On Mon, Apr 29, 2019 at 08:30:18AM +0200, Fabien COELHO wrote:
> 
> Hello David,
> 
> About v5: applies, compiles, global & local make check ok, doc gen ok.
> 
> Very minor comment: \qecho is just before \o in the embedded help, where it
> should be just after. Sorry I did not see it on the preceding submission.

Done.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

  17. Re: [PATCH v4] Add \warn to psql

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-04-29T19:39:06Z

    Hello David,

>> About v5: applies, compiles, global & local make check ok, doc gen ok.
>>
>> Very minor comment: \qecho is just before \o in the embedded help, where it
>> should be just after. Sorry I did not see it on the preceding submission.
>
> Done.

Patch v6 applies, compiles, global & local make check ok, doc gen ok.

This is okay for me, marked as ready.

-- 
Fabien.

  18. Re: [PATCH v4] Add \warn to psql

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-04-30T13:46:28Z

    >>> About v5: applies, compiles, global & local make check ok, doc gen ok.
>>> 
>>> Very minor comment: \qecho is just before \o in the embedded help, 
>>> where it should be just after. Sorry I did not see it on the preceding 
>>> submission.
>
> Unfortunately new TAP test doesn't pass on my machine. I'm not good at Perl 
> and didn't get the reason of the failure quickly.

I guess that you have a verbose ~/.psqlrc.

Can you try with adding -X to psql option when calling psql from the tap 
test?

-- 
Fabien.

  19. Re: [PATCH v4] Add \warn to psql

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2019-05-01T07:05:44Z

    (Unfortunately I accidentally sent my previous two messages using my personal
email address because of my email client configuration. This address is not
verified by PostgreSQL.org services and messages didn't reach hackers mailing
lists, so I recent latest message).

On Tue, Apr 30, 2019 at 4:46 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> > Unfortunately new TAP test doesn't pass on my machine. I'm not good at Perl
> > and didn't get the reason of the failure quickly.
>
> I guess that you have a verbose ~/.psqlrc.
>
> Can you try with adding -X to psql option when calling psql from the tap
> test?

Ah, true. This patch works for me:

diff --git a/src/bin/psql/t/001_psql.pl b/src/bin/psql/t/001_psql.pl
index 32dd43279b..637baa94c9 100644
--- a/src/bin/psql/t/001_psql.pl
+++ b/src/bin/psql/t/001_psql.pl
@@ -20,7 +20,7 @@ sub psql
  {
     local $Test::Builder::Level = $Test::Builder::Level + 1;
     my ($opts, $stat, $in, $out, $err, $name) = @_;
-   my @cmd = ('psql', split /\s+/, $opts);
+   my @cmd = ('psql', '-X', split /\s+/, $opts);
     $node->command_checks_all(\@cmd, $stat, $out, $err, $name, $in);
     return;
  }

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

  20. Re: [PATCH v4] Add \warn to psql

    David Fetter <david@fetter.org> — 2019-05-01T07:38:57Z

    On Wed, May 01, 2019 at 10:05:44AM +0300, Arthur Zakirov wrote:
> (Unfortunately I accidentally sent my previous two messages using my personal
> email address because of my email client configuration. This address is not
> verified by PostgreSQL.org services and messages didn't reach hackers mailing
> lists, so I recent latest message).
> 
> On Tue, Apr 30, 2019 at 4:46 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> > > Unfortunately new TAP test doesn't pass on my machine. I'm not good at Perl
> > > and didn't get the reason of the failure quickly.
> >
> > I guess that you have a verbose ~/.psqlrc.
> >
> > Can you try with adding -X to psql option when calling psql from the tap
> > test?
> 
> Ah, true. This patch works for me:
> 
> diff --git a/src/bin/psql/t/001_psql.pl b/src/bin/psql/t/001_psql.pl
> index 32dd43279b..637baa94c9 100644
> --- a/src/bin/psql/t/001_psql.pl
> +++ b/src/bin/psql/t/001_psql.pl
> @@ -20,7 +20,7 @@ sub psql
>   {
>      local $Test::Builder::Level = $Test::Builder::Level + 1;
>      my ($opts, $stat, $in, $out, $err, $name) = @_;
> -   my @cmd = ('psql', split /\s+/, $opts);
> +   my @cmd = ('psql', '-X', split /\s+/, $opts);
>      $node->command_checks_all(\@cmd, $stat, $out, $err, $name, $in);
>      return;
>   }

Please find attached :)

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

  21. Re: [PATCH v4] Add \warn to psql

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-05-01T10:02:48Z

    >>> I guess that you have a verbose ~/.psqlrc.
>>>
>>> Can you try with adding -X to psql option when calling psql from the tap
>>> test?
>>
>> Ah, true. This patch works for me:
>>
>> diff --git a/src/bin/psql/t/001_psql.pl b/src/bin/psql/t/001_psql.pl
>> index 32dd43279b..637baa94c9 100644
>> --- a/src/bin/psql/t/001_psql.pl
>> +++ b/src/bin/psql/t/001_psql.pl
>> @@ -20,7 +20,7 @@ sub psql
>>   {
>>      local $Test::Builder::Level = $Test::Builder::Level + 1;
>>      my ($opts, $stat, $in, $out, $err, $name) = @_;
>> -   my @cmd = ('psql', split /\s+/, $opts);
>> +   my @cmd = ('psql', '-X', split /\s+/, $opts);
>>      $node->command_checks_all(\@cmd, $stat, $out, $err, $name, $in);
>>      return;
>>   }
>
> Please find attached :)

Good. Works for me, even with a verbose .psqlrc. Switched back to ready.

-- 
Fabien.

  22. Re: [PATCH v4] Add \warn to psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-07-02T20:10:50Z

    David Fetter <david@fetter.org> writes:
> [ v7-0001-Add-warn-to-psql.patch ]

I took a look at this.  I have no quibble with the proposed feature,
and the implementation is certainly simple enough.  But I'm unconvinced
about the proposed test scaffolding.  Spinning up a new PG instance is a
*hell* of a lot of overhead to pay for testing something that could be
tested as per attached.  Admittedly, the attached doesn't positively
prove which pipe each output string went down, but that does not strike
me as a concern large enough to justify adding a TAP test for.

I'd be happier about adding TAP infrastructure if it looked like it'd
be usable to test some of the psql areas that are unreachable by the
existing test methodology, particularly tab-complete.c and prompt.c.
But I don't see anything here that looks like it'll work for that.

I don't like what you did to command_checks_all, either --- it could
hardly say "bolted on after the fact" more clearly if you'd written
that in <blink><red> text.  If we need an input-stream argument,
let's just add it in a rational place and adjust the callers.
There aren't that many of 'em, nor has the subroutine been around
all that long.

			regards, tom lane

  23. Re: [PATCH v4] Add \warn to psql

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-07-03T07:05:28Z

    Hello Tom,

> I took a look at this.  I have no quibble with the proposed feature,
> and the implementation is certainly simple enough.  But I'm unconvinced
> about the proposed test scaffolding.  Spinning up a new PG instance is a
> *hell* of a lot of overhead to pay for testing something that could be
> tested as per attached.


> Admittedly, the attached doesn't positively prove which pipe each output 
> string went down, but that does not strike me as a concern large enough 
> to justify adding a TAP test for.

Sure.

The point is that there would be at least *one* TAP tests so that many 
other features of psql, although not all, can be tested. I have been 
reviewing quite a few patches without tests because of this lack of 
infrastructure, and no one patch is ever going to justify a TAP test on 
its own. It has to start somewhere. Currently psql coverage is abysmal, 
around 40% of lines & functions are called by the whole non regression 
tests, despite the hundreds of psql-relying tests. Pg is around 80% 
coverage overall.

Basically, I really thing that one psql dedicated TAP test should be 
added, not for \warn per se, but for other features.

> I'd be happier about adding TAP infrastructure if it looked like it'd
> be usable to test some of the psql areas that are unreachable by the
> existing test methodology, particularly tab-complete.c and prompt.c.
> But I don't see anything here that looks like it'll work for that.

The tab complete and prompt are special interactive cases and probably 
require special infrastructure to make a test believe it is running 
against a tty while it is not. The point of this proposal is not to 
address these special needs, but to lay a basic infra.

> I don't like what you did to command_checks_all,

Yeah, probably my fault, not David.

> either --- it could hardly say "bolted on after the fact" more clearly 
> if you'd written that in <blink><red> text.  If we need an input-stream 
> argument, let's just add it in a rational place and adjust the callers. 
> There aren't that many of 'em, nor has the subroutine been around all 
> that long.

I wanted to avoid breaking the function signature of it is used by some 
external packages. Not caring is an option.

-- 
Fabien.

  24. Re: [PATCH v4] Add \warn to psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-07-03T14:06:08Z

    Fabien COELHO <coelho@cri.ensmp.fr> writes:
> The point is that there would be at least *one* TAP tests so that many 
> other features of psql, although not all, can be tested. I have been 
> reviewing quite a few patches without tests because of this lack of 
> infrastructure, and no one patch is ever going to justify a TAP test on 
> its own. It has to start somewhere. Currently psql coverage is abysmal, 
> around 40% of lines & functions are called by the whole non regression 
> tests, despite the hundreds of psql-relying tests.

Yeah, but the point I was trying to make is that that's mostly down to
laziness.  I see no reason that we couldn't be covering a lot of these
features in src/test/regress/sql/psql.sql, with far less overhead.
The interactive aspects of psql can't be tested that way ... but since
this patch doesn't actually provide any way to test those, it's not much
of a proof-of-concept.

IOW, the blocking factor here is not "does src/bin/psql/t/ exist",
it's "has somebody written a test that moves the coverage needle
meaningfully".  I'm not big on adding a bunch of overhead first and
just hoping somebody will do something to make it worthwhile later.

			regards, tom lane

  25. Re: [PATCH v4] Add \warn to psql

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-07-03T14:29:23Z

    Hello Tom,

>> The point is that there would be at least *one* TAP tests so that many
>> other features of psql, although not all, can be tested. [...]
>
> Yeah, but the point I was trying to make is that that's mostly down to
> laziness.

Not always.

I agree that using TAP test if another simpler option is available is not 
a good move.

However, in the current state, as soon as there is some variation a test 
is removed and coverage is lost, but they could be kept if the check could 
be against a regexp.

> I see no reason that we couldn't be covering a lot of these features in 
> src/test/regress/sql/psql.sql, with far less overhead. The interactive 
> aspects of psql can't be tested that way ... but since this patch 
> doesn't actually provide any way to test those, it's not much of a 
> proof-of-concept.

The PoC is checking against a set of regexp instead of expecting an exact 
output. Ok, it does not solve all possible test scenarii, that is life.

> IOW, the blocking factor here is not "does src/bin/psql/t/ exist",
> it's "has somebody written a test that moves the coverage needle
> meaningfully".  I'm not big on adding a bunch of overhead first and
> just hoping somebody will do something to make it worthwhile later.

I do intend to add coverage once a psql TAP test is available, as I have 
done with pgbench. Ok, some of the changes are still in the long CF queue, 
but at least pgbench coverage is around 90%.

I also intend to direct submitted patches to use the TAP infra when 
appropriate, instead of "no tests, too bad".

-- 
Fabien.

  26. Re: [PATCH v4] Add \warn to psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-07-05T16:38:02Z

    I wrote:
> David Fetter <david@fetter.org> writes:
>> [ v7-0001-Add-warn-to-psql.patch ]

> I took a look at this.  I have no quibble with the proposed feature,
> and the implementation is certainly simple enough.  But I'm unconvinced
> about the proposed test scaffolding.

I pushed this with the simplified test methodology.

While I was fooling with it I noticed that the existing code for -n
is buggy.  The documentation says clearly that only the first
argument is a candidate to be -n:

        If the first argument is an unquoted <literal>-n</literal> the trailing
        newline is not written.

but the actual implementation allows any argument to be recognized as
-n:

regression=# \echo this -n should not be -n like this
this should not be like thisregression=# 

I fixed that, but I'm wondering if we should back-patch that fix
or leave the back branches alone.

			regards, tom lane

  27. Re: [PATCH v4] Add \warn to psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-07-05T16:48:10Z

    Fabien COELHO <coelho@cri.ensmp.fr> writes:
> I agree that using TAP test if another simpler option is available is not 
> a good move.

> However, in the current state, as soon as there is some variation a test 
> is removed and coverage is lost, but they could be kept if the check could 
> be against a regexp.

I'm fairly suspicious of using TAP tests just to get a regexp match.
The thing I don't like about TAP tests for this is that they won't
notice if the test case prints extra stuff beyond what you were
expecting --- at least, not without care that I don't think we usually
take.

I've thought for some time that we should steal an idea from MySQL
and extend pg_regress so that individual lines of an expected-file
could have regexp match patterns rather than being just exact matches.
I'm not really sure how to do that without reimplementing diff(1)
for ourselves :-(, but that would be a very large step forward if
we could find a reasonable implementation.

Anyway, my opinion about having TAP test(s) for psql remains that
it'll be a good idea as soon as somebody submits a test that adds
a meaningful amount of code coverage that way (and the coverage
can't be gotten more simply).  But we don't need a patch that is
just trying to get the camel's nose under the tent.

			regards, tom lane

  28. Re: [PATCH v4] Add \warn to psql

    David Fetter <david@fetter.org> — 2019-07-05T21:29:03Z

    On Fri, Jul 05, 2019 at 12:38:02PM -0400, Tom Lane wrote:
> I wrote:
> > David Fetter <david@fetter.org> writes:
> >> [ v7-0001-Add-warn-to-psql.patch ]
> 
> > I took a look at this.  I have no quibble with the proposed feature,
> > and the implementation is certainly simple enough.  But I'm unconvinced
> > about the proposed test scaffolding.
> 
> I pushed this with the simplified test methodology.

Thanks!

> While I was fooling with it I noticed that the existing code for -n
> is buggy.  The documentation says clearly that only the first
> argument is a candidate to be -n:
> 
>         If the first argument is an unquoted <literal>-n</literal> the trailing
>         newline is not written.
> 
> but the actual implementation allows any argument to be recognized as
> -n:
> 
> regression=# \echo this -n should not be -n like this
> this should not be like thisregression=# 
> 
> I fixed that, but I'm wondering if we should back-patch that fix
> or leave the back branches alone.

+0.5 for back-patching.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

  29. Re: [PATCH v4] Add \warn to psql

    Bruce Momjian <bruce@momjian.us> — 2019-07-09T01:29:17Z

    On Fri, Jul  5, 2019 at 11:29:03PM +0200, David Fetter wrote:
> > While I was fooling with it I noticed that the existing code for -n
> > is buggy.  The documentation says clearly that only the first
> > argument is a candidate to be -n:
> > 
> >         If the first argument is an unquoted <literal>-n</literal> the trailing
> >         newline is not written.
> > 
> > but the actual implementation allows any argument to be recognized as
> > -n:
> > 
> > regression=# \echo this -n should not be -n like this
> > this should not be like thisregression=# 
> > 
> > I fixed that, but I'm wondering if we should back-patch that fix
> > or leave the back branches alone.
> 
> +0.5 for back-patching.

Uh, if this was done in a major release I am thinking we have to mention
this as an incompatibility, which means we should probably not backpatch
it.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

  30. Re: [PATCH v4] Add \warn to psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-07-09T03:29:00Z

    Bruce Momjian <bruce@momjian.us> writes:
> On Fri, Jul  5, 2019 at 11:29:03PM +0200, David Fetter wrote:
>>> I fixed that, but I'm wondering if we should back-patch that fix
>>> or leave the back branches alone.

>> +0.5 for back-patching.

> Uh, if this was done in a major release I am thinking we have to mention
> this as an incompatibility, which means we should probably not backpatch
> it.

How is "clearly doesn't match the documentation" not a bug?

			regards, tom lane

  31. Re: [PATCH v4] Add \warn to psql

    Bruce Momjian <bruce@momjian.us> — 2019-07-09T03:35:28Z

    On Mon, Jul  8, 2019 at 11:29:00PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > On Fri, Jul  5, 2019 at 11:29:03PM +0200, David Fetter wrote:
> >>> I fixed that, but I'm wondering if we should back-patch that fix
> >>> or leave the back branches alone.
> 
> >> +0.5 for back-patching.
> 
> > Uh, if this was done in a major release I am thinking we have to mention
> > this as an incompatibility, which means we should probably not backpatch
> > it.
> 
> How is "clearly doesn't match the documentation" not a bug?

Uh, it is a bug, but people might be expecting the existing behavior
without consulting the documentation, and we don't expect people to be
testing minor releases.

Anyway, it seems to be have been applied only to head so far.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

  32. Re: [PATCH v4] Add \warn to psql

    David G. Johnston <david.g.johnston@gmail.com> — 2019-07-09T04:19:03Z

    On Mon, Jul 8, 2019 at 8:35 PM Bruce Momjian <bruce@momjian.us> wrote:

> On Mon, Jul  8, 2019 at 11:29:00PM -0400, Tom Lane wrote:
> > Bruce Momjian <bruce@momjian.us> writes:
> > > On Fri, Jul  5, 2019 at 11:29:03PM +0200, David Fetter wrote:
> > >>> I fixed that, but I'm wondering if we should back-patch that fix
> > >>> or leave the back branches alone.
> >
> > >> +0.5 for back-patching.
> >
> > > Uh, if this was done in a major release I am thinking we have to
> mention
> > > this as an incompatibility, which means we should probably not
> backpatch
> > > it.
> >
> > How is "clearly doesn't match the documentation" not a bug?
>
> Uh, it is a bug, but people might be expecting the existing behavior
> without consulting the documentation, and we don't expect people to be
> testing minor releases.
>
> Anyway, it seems to be have been applied only to head so far.
>

I would leave it at that.  Won't Fix for released versions (neither code
nor documentation) as we describe the intended usage so people do the right
thing (which is highly likely anyway - though something like "\echo
:content_to_echo -n" wouldn't surprise me) but those that learned through
trial and error only experience a behavior change on a major release as
they would expect.  This doesn't seem important enough to warrant breaking
the general rule.  Though I'd give a +1 to v12; at least for me Beta is
generally fair game.

David J.