[PATCH v1] Add \echo_stderr to psql
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
Attachments:
v1-0001-Add-echo_stderr-to-psql.patchtext/x-diff; charset=us-asciiDownload
From 089d48c00ffa6dfd0a6a443e83f6d618c9847deb Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Sun, 21 Apr 2019 11:08:40 -0700
Subject: [PATCH v1] Add \echo_stderr to psql
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.20.1"
This is a multi-part message in MIME format.
--------------2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
- Does what it says on the label
- Do we have any way to test this?
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index b86764003d..f8481f2e1b 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1879,6 +1879,26 @@ Tue Oct 26 21:40:57 CEST 1999
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>\echo_stderr <replaceable class="parameter">text</replaceable> [ ... ]</literal></term>
+ <listitem>
+ <para>
+ Prints the arguments to the standard error, separated by one
+ space and followed by a newline. This can be useful to
+ intersperse information in the output of scripts when not polluting
+ standard output is desired. For example:
+
+<programlisting>
+=> <userinput>\echo :variable</userinput>
+=> <userinput>\echo_stderr `date`</userinput>
+Sun Apr 21 10:48:11 PDT 2019
+</programlisting>
+ If the first argument is an unquoted <literal>-n</literal> the trailing
+ newline is not written.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>\ef <optional> <replaceable class="parameter">function_description</replaceable> <optional> <replaceable class="parameter">line_number</replaceable> </optional> </optional> </literal></term>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 8254d61099..6134c43938 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -319,7 +319,7 @@ exec_command(const char *cmd,
status = exec_command_ef_ev(scan_state, active_branch, query_buf, true);
else if (strcmp(cmd, "ev") == 0)
status = exec_command_ef_ev(scan_state, active_branch, query_buf, false);
- else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "qecho") == 0)
+ else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "echo_stderr") == 0 || strcmp(cmd, "qecho") == 0)
status = exec_command_echo(scan_state, active_branch, cmd);
else if (strcmp(cmd, "elif") == 0)
status = exec_command_elif(scan_state, cstack, query_buf);
@@ -1114,7 +1114,7 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch,
}
/*
- * \echo and \qecho -- echo arguments to stdout or query output
+ * \echo, \echo_stderr and \qecho -- echo arguments to stdout or query output
*/
static backslashResult
exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd)
@@ -1129,6 +1129,8 @@ exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd)
if (strcmp(cmd, "qecho") == 0)
fout = pset.queryFout;
+ else if (strcmp(cmd, "echo_stderr") == 0)
+ fout = stderr;
else
fout = stdout;
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index d6d41b51d5..b6ef98d00c 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -207,6 +207,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _("Input/Output\n"));
fprintf(output, _(" \\copy ... perform SQL COPY with data stream to the client host\n"));
fprintf(output, _(" \\echo [STRING] write string to standard output\n"));
+ fprintf(output, _(" \\echo_stderr [STRING] write string to standard error\n"));
fprintf(output, _(" \\i FILE execute commands from file\n"));
fprintf(output, _(" \\ir FILE as \\i, but relative to location of current script\n"));
fprintf(output, _(" \\o [FILE] send all query results to file or |pipe\n"));
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index bcddc7601e..a76be5f068 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1406,7 +1406,7 @@ psql_completion(const char *text, int start, int end)
"\\dm", "\\dn", "\\do", "\\dO", "\\dp", "\\dP", "\\dPi", "\\dPt",
"\\drds", "\\dRs", "\\dRp", "\\ds", "\\dS",
"\\dt", "\\dT", "\\dv", "\\du", "\\dx", "\\dy",
- "\\e", "\\echo", "\\ef", "\\elif", "\\else", "\\encoding",
+ "\\e", "\\echo", "\\echo_stderr", "\\ef", "\\elif", "\\else", "\\encoding",
"\\endif", "\\errverbose", "\\ev",
"\\f",
"\\g", "\\gdesc", "\\gexec", "\\gset", "\\gx",
--------------2.20.1--
ne 21. 4. 2019 v 20:31 odesílatel David Fetter <david@fetter.org> napsal:
Folks,
Any interest in this?
has sense
Pavel
Show quoted text
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Any interest in this?
Yep, although I'm not sure of the suggested command name. More
suggestions:
\stderr ...
\err ...
\error ...
\warn ...
\warning ...
--
Fabien.
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]The others are cache coherency and off-by-one -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 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
--
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
\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.
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.
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
Attachments:
v2-0001-Add-warn-to-psql.patchtext/x-diff; charset=us-asciiDownload
From db987e21d0e25ce306d5482fa78d71de8454f1aa Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Sun, 21 Apr 2019 11:08:40 -0700
Subject: [PATCH v2] Add \warn to psql
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.20.1"
This is a multi-part message in MIME format.
--------------2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
- Does what it says on the label
- Do we have any way to test this?
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index b86764003d..4edf8e67a1 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1879,6 +1879,26 @@ Tue Oct 26 21:40:57 CEST 1999
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>\warn <replaceable class="parameter">text</replaceable> [ ... ]</literal></term>
+ <listitem>
+ <para>
+ Prints the arguments to the standard error, separated by one
+ space and followed by a newline. This can be useful to
+ intersperse information in the output of scripts when not polluting
+ standard output is desired. For example:
+
+<programlisting>
+=> <userinput>\echo :variable</userinput>
+=> <userinput>\warn `date`</userinput>
+Sun Apr 21 10:48:11 PDT 2019
+</programlisting>
+ If the first argument is an unquoted <literal>-n</literal> the trailing
+ newline is not written.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>\ef <optional> <replaceable class="parameter">function_description</replaceable> <optional> <replaceable class="parameter">line_number</replaceable> </optional> </optional> </literal></term>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 8254d61099..9425f02005 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -319,7 +319,7 @@ exec_command(const char *cmd,
status = exec_command_ef_ev(scan_state, active_branch, query_buf, true);
else if (strcmp(cmd, "ev") == 0)
status = exec_command_ef_ev(scan_state, active_branch, query_buf, false);
- else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "qecho") == 0)
+ else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "warn") == 0 || strcmp(cmd, "qecho") == 0)
status = exec_command_echo(scan_state, active_branch, cmd);
else if (strcmp(cmd, "elif") == 0)
status = exec_command_elif(scan_state, cstack, query_buf);
@@ -1114,7 +1114,7 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch,
}
/*
- * \echo and \qecho -- echo arguments to stdout or query output
+ * \echo, \warn and \qecho -- echo arguments to stdout or query output
*/
static backslashResult
exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd)
@@ -1129,6 +1129,8 @@ exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd)
if (strcmp(cmd, "qecho") == 0)
fout = pset.queryFout;
+ else if (strcmp(cmd, "warn") == 0)
+ fout = stderr;
else
fout = stdout;
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index d6d41b51d5..2ecb8b73c1 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -207,6 +207,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _("Input/Output\n"));
fprintf(output, _(" \\copy ... perform SQL COPY with data stream to the client host\n"));
fprintf(output, _(" \\echo [STRING] write string to standard output\n"));
+ fprintf(output, _(" \\warn [STRING] write string to standard error\n"));
fprintf(output, _(" \\i FILE execute commands from file\n"));
fprintf(output, _(" \\ir FILE as \\i, but relative to location of current script\n"));
fprintf(output, _(" \\o [FILE] send all query results to file or |pipe\n"));
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index bcddc7601e..383a17a7f4 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1421,7 +1421,7 @@ psql_completion(const char *text, int start, int end)
"\\t", "\\T", "\\timing",
"\\unset",
"\\x",
- "\\w", "\\watch",
+ "\\w","\\warn", "\\watch",
"\\z",
"\\!", "\\?",
NULL
--------------2.20.1--
\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.
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.
Attachments:
psql-warn-2-tap.patchtext/x-diff; name=psql-warn-2-tap.patchDownload
diff --git a/src/bin/psql/.gitignore b/src/bin/psql/.gitignore
index c2862b12d6..d324c1c1fa 100644
--- a/src/bin/psql/.gitignore
+++ b/src/bin/psql/.gitignore
@@ -3,3 +3,4 @@
/sql_help.c
/psql
+/tmp_check
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index 69bb297fe7..9473ab01cb 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -60,8 +60,15 @@ uninstall:
clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+ rm -rf tmp_check
# files removed here are supposed to be in the distribution tarball,
# so do not clean them in the clean/distclean rules
maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
diff --git a/src/bin/psql/t/001_psql.pl b/src/bin/psql/t/001_psql.pl
new file mode 100644
index 0000000000..32dd43279b
--- /dev/null
+++ b/src/bin/psql/t/001_psql.pl
@@ -0,0 +1,32 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More;
+
+my $node = get_new_node('main');
+$node->init();
+$node->start();
+
+# invoke psql
+# - opts: space-separated options and arguments
+# - stat: expected exit status
+# - in: input stream
+# - out: list of re to check on stdout
+# - err: list of re to check on stderr
+# - name: of the test
+sub psql
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+ my ($opts, $stat, $in, $out, $err, $name) = @_;
+ my @cmd = ('psql', split /\s+/, $opts);
+ $node->command_checks_all(\@cmd, $stat, $out, $err, $name, $in);
+ return;
+}
+
+psql('-c \\q', 0, '', [ qr{^$} ], [ qr{^$} ], 'psql -c');
+psql('', 0,"\\echo hello\n\\warn world\n\\q\n", [ qr{^hello$} ], [ qr{^world$} ], 'psql in/out/err');
+
+$node->stop();
+done_testing();
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index a164cdbd8c..6ad7f681ae 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -519,22 +519,24 @@ sub command_fails_like
}
# Run a command and check its status and outputs.
-# The 5 arguments are:
+# The 5 to 6 arguments are:
# - cmd: ref to list for command, options and arguments to run
# - ret: expected exit status
# - out: ref to list of re to be checked against stdout (all must match)
# - err: ref to list of re to be checked against stderr (all must match)
# - test_name: name of test
+# - in: standard input
sub command_checks_all
{
local $Test::Builder::Level = $Test::Builder::Level + 1;
- my ($cmd, $expected_ret, $out, $err, $test_name) = @_;
+ my ($cmd, $expected_ret, $out, $err, $test_name, $in) = @_;
+ $in = '' if not defined $in;
# run command
my ($stdout, $stderr);
print("# Running: " . join(" ", @{$cmd}) . "\n");
- IPC::Run::run($cmd, '>', \$stdout, '2>', \$stderr);
+ IPC::Run::run($cmd, '<', \$in, '>', \$stdout, '2>', \$stderr);
# See http://perldoc.perl.org/perlvar.html#%24CHILD_ERROR
my $ret = $?;
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
Attachments:
v3-0001-Add-warn-to-psql.patchtext/x-diff; charset=us-asciiDownload
From 2f7cbf9980c5ce1049728a0e2a3444cbd106f253 Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Sun, 21 Apr 2019 11:08:40 -0700
Subject: [PATCH v3] Add \warn to psql
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.20.1"
This is a multi-part message in MIME format.
--------------2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
- Does what it says on the label
- Adds TAP tests for same
- In passing, expose the -n option for \echo, \qecho, and \warn in \? output
create mode 100644 src/bin/psql/t/001_psql.pl
--------------2.20.1
Content-Type: text/x-patch; name="v3-0001-Add-warn-to-psql.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="v3-0001-Add-warn-to-psql.patch"
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index b86764003d..4edf8e67a1 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1879,6 +1879,26 @@ Tue Oct 26 21:40:57 CEST 1999
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>\warn <replaceable class="parameter">text</replaceable> [ ... ]</literal></term>
+ <listitem>
+ <para>
+ Prints the arguments to the standard error, separated by one
+ space and followed by a newline. This can be useful to
+ intersperse information in the output of scripts when not polluting
+ standard output is desired. For example:
+
+<programlisting>
+=> <userinput>\echo :variable</userinput>
+=> <userinput>\warn `date`</userinput>
+Sun Apr 21 10:48:11 PDT 2019
+</programlisting>
+ If the first argument is an unquoted <literal>-n</literal> the trailing
+ newline is not written.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>\ef <optional> <replaceable class="parameter">function_description</replaceable> <optional> <replaceable class="parameter">line_number</replaceable> </optional> </optional> </literal></term>
diff --git a/src/bin/psql/.gitignore b/src/bin/psql/.gitignore
index c2862b12d6..d324c1c1fa 100644
--- a/src/bin/psql/.gitignore
+++ b/src/bin/psql/.gitignore
@@ -3,3 +3,4 @@
/sql_help.c
/psql
+/tmp_check
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index 69bb297fe7..9473ab01cb 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -60,8 +60,15 @@ uninstall:
clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+ rm -rf tmp_check
# files removed here are supposed to be in the distribution tarball,
# so do not clean them in the clean/distclean rules
maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 8254d61099..9425f02005 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -319,7 +319,7 @@ exec_command(const char *cmd,
status = exec_command_ef_ev(scan_state, active_branch, query_buf, true);
else if (strcmp(cmd, "ev") == 0)
status = exec_command_ef_ev(scan_state, active_branch, query_buf, false);
- else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "qecho") == 0)
+ else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "warn") == 0 || strcmp(cmd, "qecho") == 0)
status = exec_command_echo(scan_state, active_branch, cmd);
else if (strcmp(cmd, "elif") == 0)
status = exec_command_elif(scan_state, cstack, query_buf);
@@ -1114,7 +1114,7 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch,
}
/*
- * \echo and \qecho -- echo arguments to stdout or query output
+ * \echo, \warn and \qecho -- echo arguments to stdout or query output
*/
static backslashResult
exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd)
@@ -1129,6 +1129,8 @@ exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd)
if (strcmp(cmd, "qecho") == 0)
fout = pset.queryFout;
+ else if (strcmp(cmd, "warn") == 0)
+ fout = stderr;
else
fout = stdout;
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index d6d41b51d5..434388608a 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -206,11 +206,12 @@ slashUsage(unsigned short int pager)
fprintf(output, _("Input/Output\n"));
fprintf(output, _(" \\copy ... perform SQL COPY with data stream to the client host\n"));
- fprintf(output, _(" \\echo [STRING] write string to standard output\n"));
+ fprintf(output, _(" \\echo [-n] [STRING] write string to standard output (-n leaves off newline)\n"));
+ fprintf(output, _(" \\qecho [-n] [STRING] write string to query output stream (see \\o)\n"));
+ fprintf(output, _(" \\warn [-n] [STRING] write string to standard error\n"));
fprintf(output, _(" \\i FILE execute commands from file\n"));
fprintf(output, _(" \\ir FILE as \\i, but relative to location of current script\n"));
fprintf(output, _(" \\o [FILE] send all query results to file or |pipe\n"));
- fprintf(output, _(" \\qecho [STRING] write string to query output stream (see \\o)\n"));
fprintf(output, "\n");
fprintf(output, _("Conditional\n"));
diff --git a/src/bin/psql/t/001_psql.pl b/src/bin/psql/t/001_psql.pl
new file mode 100644
index 0000000000..32dd43279b
--- /dev/null
+++ b/src/bin/psql/t/001_psql.pl
@@ -0,0 +1,32 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More;
+
+my $node = get_new_node('main');
+$node->init();
+$node->start();
+
+# invoke psql
+# - opts: space-separated options and arguments
+# - stat: expected exit status
+# - in: input stream
+# - out: list of re to check on stdout
+# - err: list of re to check on stderr
+# - name: of the test
+sub psql
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+ my ($opts, $stat, $in, $out, $err, $name) = @_;
+ my @cmd = ('psql', split /\s+/, $opts);
+ $node->command_checks_all(\@cmd, $stat, $out, $err, $name, $in);
+ return;
+}
+
+psql('-c \\q', 0, '', [ qr{^$} ], [ qr{^$} ], 'psql -c');
+psql('', 0,"\\echo hello\n\\warn world\n\\q\n", [ qr{^hello$} ], [ qr{^world$} ], 'psql in/out/err');
+
+$node->stop();
+done_testing();
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index bcddc7601e..383a17a7f4 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1421,7 +1421,7 @@ psql_completion(const char *text, int start, int end)
"\\t", "\\T", "\\timing",
"\\unset",
"\\x",
- "\\w", "\\watch",
+ "\\w","\\warn", "\\watch",
"\\z",
"\\!", "\\?",
NULL
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index a164cdbd8c..6ad7f681ae 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -519,22 +519,24 @@ sub command_fails_like
}
# Run a command and check its status and outputs.
-# The 5 arguments are:
+# The 5 to 6 arguments are:
# - cmd: ref to list for command, options and arguments to run
# - ret: expected exit status
# - out: ref to list of re to be checked against stdout (all must match)
# - err: ref to list of re to be checked against stderr (all must match)
# - test_name: name of test
+# - in: standard input
sub command_checks_all
{
local $Test::Builder::Level = $Test::Builder::Level + 1;
- my ($cmd, $expected_ret, $out, $err, $test_name) = @_;
+ my ($cmd, $expected_ret, $out, $err, $test_name, $in) = @_;
+ $in = '' if not defined $in;
# run command
my ($stdout, $stderr);
print("# Running: " . join(" ", @{$cmd}) . "\n");
- IPC::Run::run($cmd, '>', \$stdout, '2>', \$stderr);
+ IPC::Run::run($cmd, '<', \$in, '>', \$stdout, '2>', \$stderr);
# See http://perldoc.perl.org/perlvar.html#%24CHILD_ERROR
my $ret = $?;
--------------2.20.1--
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.
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
Attachments:
v4-0001-Add-warn-to-psql.patchtext/x-diff; charset=us-asciiDownload
From 76ce9a85f92ec4f2283ee967b3067e3f3efda232 Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Sun, 21 Apr 2019 11:08:40 -0700
Subject: [PATCH v4] Add \warn to psql
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.20.1"
This is a multi-part message in MIME format.
--------------2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
- Does what it says on the label
- Adds TAP tests for same
- In passing, expose the -n option for \echo, \qecho, and \warn in \? output
create mode 100644 src/bin/psql/t/001_psql.pl
--------------2.20.1
Content-Type: text/x-patch; name="v4-0001-Add-warn-to-psql.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="v4-0001-Add-warn-to-psql.patch"
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index b86764003d..4edf8e67a1 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1879,6 +1879,26 @@ Tue Oct 26 21:40:57 CEST 1999
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>\warn <replaceable class="parameter">text</replaceable> [ ... ]</literal></term>
+ <listitem>
+ <para>
+ Prints the arguments to the standard error, separated by one
+ space and followed by a newline. This can be useful to
+ intersperse information in the output of scripts when not polluting
+ standard output is desired. For example:
+
+<programlisting>
+=> <userinput>\echo :variable</userinput>
+=> <userinput>\warn `date`</userinput>
+Sun Apr 21 10:48:11 PDT 2019
+</programlisting>
+ If the first argument is an unquoted <literal>-n</literal> the trailing
+ newline is not written.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>\ef <optional> <replaceable class="parameter">function_description</replaceable> <optional> <replaceable class="parameter">line_number</replaceable> </optional> </optional> </literal></term>
diff --git a/src/bin/psql/.gitignore b/src/bin/psql/.gitignore
index c2862b12d6..d324c1c1fa 100644
--- a/src/bin/psql/.gitignore
+++ b/src/bin/psql/.gitignore
@@ -3,3 +3,4 @@
/sql_help.c
/psql
+/tmp_check
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index 69bb297fe7..9473ab01cb 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -60,8 +60,15 @@ uninstall:
clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+ rm -rf tmp_check
# files removed here are supposed to be in the distribution tarball,
# so do not clean them in the clean/distclean rules
maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 8254d61099..d4846ce729 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -319,7 +319,7 @@ exec_command(const char *cmd,
status = exec_command_ef_ev(scan_state, active_branch, query_buf, true);
else if (strcmp(cmd, "ev") == 0)
status = exec_command_ef_ev(scan_state, active_branch, query_buf, false);
- else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "qecho") == 0)
+ else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "qecho") == 0 || strcmp(cmd, "warn") == 0)
status = exec_command_echo(scan_state, active_branch, cmd);
else if (strcmp(cmd, "elif") == 0)
status = exec_command_elif(scan_state, cstack, query_buf);
@@ -1114,7 +1114,7 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch,
}
/*
- * \echo and \qecho -- echo arguments to stdout or query output
+ * \echo, \warn and \qecho -- echo arguments to stdout or query output
*/
static backslashResult
exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd)
@@ -1129,6 +1129,8 @@ exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd)
if (strcmp(cmd, "qecho") == 0)
fout = pset.queryFout;
+ else if (strcmp(cmd, "warn") == 0)
+ fout = stderr;
else
fout = stdout;
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index d6d41b51d5..bc98f01be7 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -206,11 +206,12 @@ slashUsage(unsigned short int pager)
fprintf(output, _("Input/Output\n"));
fprintf(output, _(" \\copy ... perform SQL COPY with data stream to the client host\n"));
- fprintf(output, _(" \\echo [STRING] write string to standard output\n"));
+ fprintf(output, _(" \\echo [-n] [STRING] write string to standard output (-n for no newline)\n"));
fprintf(output, _(" \\i FILE execute commands from file\n"));
fprintf(output, _(" \\ir FILE as \\i, but relative to location of current script\n"));
+ fprintf(output, _(" \\qecho [-n] [STRING] write string to query output stream (-n for no newline, see \\o)\n"));
fprintf(output, _(" \\o [FILE] send all query results to file or |pipe\n"));
- fprintf(output, _(" \\qecho [STRING] write string to query output stream (see \\o)\n"));
+ fprintf(output, _(" \\warn [-n] [STRING] write string to standard error (-n for no newline)\n"));
fprintf(output, "\n");
fprintf(output, _("Conditional\n"));
diff --git a/src/bin/psql/t/001_psql.pl b/src/bin/psql/t/001_psql.pl
new file mode 100644
index 0000000000..32dd43279b
--- /dev/null
+++ b/src/bin/psql/t/001_psql.pl
@@ -0,0 +1,32 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More;
+
+my $node = get_new_node('main');
+$node->init();
+$node->start();
+
+# invoke psql
+# - opts: space-separated options and arguments
+# - stat: expected exit status
+# - in: input stream
+# - out: list of re to check on stdout
+# - err: list of re to check on stderr
+# - name: of the test
+sub psql
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+ my ($opts, $stat, $in, $out, $err, $name) = @_;
+ my @cmd = ('psql', split /\s+/, $opts);
+ $node->command_checks_all(\@cmd, $stat, $out, $err, $name, $in);
+ return;
+}
+
+psql('-c \\q', 0, '', [ qr{^$} ], [ qr{^$} ], 'psql -c');
+psql('', 0,"\\echo hello\n\\warn world\n\\q\n", [ qr{^hello$} ], [ qr{^world$} ], 'psql in/out/err');
+
+$node->stop();
+done_testing();
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index bcddc7601e..383a17a7f4 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1421,7 +1421,7 @@ psql_completion(const char *text, int start, int end)
"\\t", "\\T", "\\timing",
"\\unset",
"\\x",
- "\\w", "\\watch",
+ "\\w","\\warn", "\\watch",
"\\z",
"\\!", "\\?",
NULL
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index a164cdbd8c..6ad7f681ae 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -519,22 +519,24 @@ sub command_fails_like
}
# Run a command and check its status and outputs.
-# The 5 arguments are:
+# The 5 to 6 arguments are:
# - cmd: ref to list for command, options and arguments to run
# - ret: expected exit status
# - out: ref to list of re to be checked against stdout (all must match)
# - err: ref to list of re to be checked against stderr (all must match)
# - test_name: name of test
+# - in: standard input
sub command_checks_all
{
local $Test::Builder::Level = $Test::Builder::Level + 1;
- my ($cmd, $expected_ret, $out, $err, $test_name) = @_;
+ my ($cmd, $expected_ret, $out, $err, $test_name, $in) = @_;
+ $in = '' if not defined $in;
# run command
my ($stdout, $stderr);
print("# Running: " . join(" ", @{$cmd}) . "\n");
- IPC::Run::run($cmd, '>', \$stdout, '2>', \$stderr);
+ IPC::Run::run($cmd, '<', \$in, '>', \$stdout, '2>', \$stderr);
# See http://perldoc.perl.org/perlvar.html#%24CHILD_ERROR
my $ret = $?;
--------------2.20.1--
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.
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
Attachments:
v5-0001-Add-warn-to-psql.patchtext/x-diff; charset=us-asciiDownload
From ede29ec28e833c2c86638bbc08f3400fe6f7f3f9 Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Sun, 21 Apr 2019 11:08:40 -0700
Subject: [PATCH v5] Add \warn to psql
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.20.1"
This is a multi-part message in MIME format.
--------------2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
- Does what it says on the label
- Adds TAP tests for same
- In passing, expose the -n option for \echo, \qecho, and \warn in \? output
create mode 100644 src/bin/psql/t/001_psql.pl
--------------2.20.1
Content-Type: text/x-patch; name="v5-0001-Add-warn-to-psql.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="v5-0001-Add-warn-to-psql.patch"
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index b86764003d..e474efb521 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3220,6 +3220,25 @@ testdb=> <userinput>\setenv LESS -imx4F</userinput>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>\warn <replaceable class="parameter">text</replaceable> [ ... ]</literal></term>
+ <listitem>
+ <para>
+ Prints the arguments to the standard error, separated by one
+ space and followed by a newline. This can be useful to
+ intersperse information in the output of scripts when not polluting
+ standard output is desired. For example:
+
+<programlisting>
+=> <userinput>\warn `date`</userinput>
+Sun Apr 28 21:00:10 PDT 2019
+</programlisting>
+ If the first argument is an unquoted <literal>-n</literal> the trailing
+ newline is not written.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>\watch [ <replaceable class="parameter">seconds</replaceable> ]</literal></term>
diff --git a/src/bin/psql/.gitignore b/src/bin/psql/.gitignore
index c2862b12d6..d324c1c1fa 100644
--- a/src/bin/psql/.gitignore
+++ b/src/bin/psql/.gitignore
@@ -3,3 +3,4 @@
/sql_help.c
/psql
+/tmp_check
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index 69bb297fe7..9473ab01cb 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -60,8 +60,15 @@ uninstall:
clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+ rm -rf tmp_check
# files removed here are supposed to be in the distribution tarball,
# so do not clean them in the clean/distclean rules
maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 8254d61099..f6fedb45b7 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -319,7 +319,7 @@ exec_command(const char *cmd,
status = exec_command_ef_ev(scan_state, active_branch, query_buf, true);
else if (strcmp(cmd, "ev") == 0)
status = exec_command_ef_ev(scan_state, active_branch, query_buf, false);
- else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "qecho") == 0)
+ else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "qecho") == 0 || strcmp(cmd, "warn") == 0)
status = exec_command_echo(scan_state, active_branch, cmd);
else if (strcmp(cmd, "elif") == 0)
status = exec_command_elif(scan_state, cstack, query_buf);
@@ -1114,7 +1114,7 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch,
}
/*
- * \echo and \qecho -- echo arguments to stdout or query output
+ * \echo, \qecho, and \warn -- echo arguments to stdout, query output, or stderr
*/
static backslashResult
exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd)
@@ -1129,6 +1129,8 @@ exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd)
if (strcmp(cmd, "qecho") == 0)
fout = pset.queryFout;
+ else if (strcmp(cmd, "warn") == 0)
+ fout = stderr;
else
fout = stdout;
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index d6d41b51d5..bc98f01be7 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -206,11 +206,12 @@ slashUsage(unsigned short int pager)
fprintf(output, _("Input/Output\n"));
fprintf(output, _(" \\copy ... perform SQL COPY with data stream to the client host\n"));
- fprintf(output, _(" \\echo [STRING] write string to standard output\n"));
+ fprintf(output, _(" \\echo [-n] [STRING] write string to standard output (-n for no newline)\n"));
fprintf(output, _(" \\i FILE execute commands from file\n"));
fprintf(output, _(" \\ir FILE as \\i, but relative to location of current script\n"));
+ fprintf(output, _(" \\qecho [-n] [STRING] write string to query output stream (-n for no newline, see \\o)\n"));
fprintf(output, _(" \\o [FILE] send all query results to file or |pipe\n"));
- fprintf(output, _(" \\qecho [STRING] write string to query output stream (see \\o)\n"));
+ fprintf(output, _(" \\warn [-n] [STRING] write string to standard error (-n for no newline)\n"));
fprintf(output, "\n");
fprintf(output, _("Conditional\n"));
diff --git a/src/bin/psql/t/001_psql.pl b/src/bin/psql/t/001_psql.pl
new file mode 100644
index 0000000000..32dd43279b
--- /dev/null
+++ b/src/bin/psql/t/001_psql.pl
@@ -0,0 +1,32 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More;
+
+my $node = get_new_node('main');
+$node->init();
+$node->start();
+
+# invoke psql
+# - opts: space-separated options and arguments
+# - stat: expected exit status
+# - in: input stream
+# - out: list of re to check on stdout
+# - err: list of re to check on stderr
+# - name: of the test
+sub psql
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+ my ($opts, $stat, $in, $out, $err, $name) = @_;
+ my @cmd = ('psql', split /\s+/, $opts);
+ $node->command_checks_all(\@cmd, $stat, $out, $err, $name, $in);
+ return;
+}
+
+psql('-c \\q', 0, '', [ qr{^$} ], [ qr{^$} ], 'psql -c');
+psql('', 0,"\\echo hello\n\\warn world\n\\q\n", [ qr{^hello$} ], [ qr{^world$} ], 'psql in/out/err');
+
+$node->stop();
+done_testing();
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index bcddc7601e..383a17a7f4 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1421,7 +1421,7 @@ psql_completion(const char *text, int start, int end)
"\\t", "\\T", "\\timing",
"\\unset",
"\\x",
- "\\w", "\\watch",
+ "\\w","\\warn", "\\watch",
"\\z",
"\\!", "\\?",
NULL
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index a164cdbd8c..6ad7f681ae 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -519,22 +519,24 @@ sub command_fails_like
}
# Run a command and check its status and outputs.
-# The 5 arguments are:
+# The 5 to 6 arguments are:
# - cmd: ref to list for command, options and arguments to run
# - ret: expected exit status
# - out: ref to list of re to be checked against stdout (all must match)
# - err: ref to list of re to be checked against stderr (all must match)
# - test_name: name of test
+# - in: standard input
sub command_checks_all
{
local $Test::Builder::Level = $Test::Builder::Level + 1;
- my ($cmd, $expected_ret, $out, $err, $test_name) = @_;
+ my ($cmd, $expected_ret, $out, $err, $test_name, $in) = @_;
+ $in = '' if not defined $in;
# run command
my ($stdout, $stderr);
print("# Running: " . join(" ", @{$cmd}) . "\n");
- IPC::Run::run($cmd, '>', \$stdout, '2>', \$stderr);
+ IPC::Run::run($cmd, '<', \$in, '>', \$stdout, '2>', \$stderr);
# See http://perldoc.perl.org/perlvar.html#%24CHILD_ERROR
my $ret = $?;
--------------2.20.1--
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.
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
Attachments:
v6-0001-Add-warn-to-psql.patchtext/x-diff; charset=us-asciiDownload
From c2cc57537124edac1a0ec5ae2d991d94529c8bc0 Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Sun, 21 Apr 2019 11:08:40 -0700
Subject: [PATCH v6] Add \warn to psql
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.20.1"
This is a multi-part message in MIME format.
--------------2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
- Does what it says on the label
- Adds TAP tests for same
- In passing, expose the -n option for \echo, \qecho, and \warn in \? output
create mode 100644 src/bin/psql/t/001_psql.pl
--------------2.20.1
Content-Type: text/x-patch; name="v6-0001-Add-warn-to-psql.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="v6-0001-Add-warn-to-psql.patch"
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index b86764003d..e474efb521 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3220,6 +3220,25 @@ testdb=> <userinput>\setenv LESS -imx4F</userinput>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>\warn <replaceable class="parameter">text</replaceable> [ ... ]</literal></term>
+ <listitem>
+ <para>
+ Prints the arguments to the standard error, separated by one
+ space and followed by a newline. This can be useful to
+ intersperse information in the output of scripts when not polluting
+ standard output is desired. For example:
+
+<programlisting>
+=> <userinput>\warn `date`</userinput>
+Sun Apr 28 21:00:10 PDT 2019
+</programlisting>
+ If the first argument is an unquoted <literal>-n</literal> the trailing
+ newline is not written.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>\watch [ <replaceable class="parameter">seconds</replaceable> ]</literal></term>
diff --git a/src/bin/psql/.gitignore b/src/bin/psql/.gitignore
index c2862b12d6..d324c1c1fa 100644
--- a/src/bin/psql/.gitignore
+++ b/src/bin/psql/.gitignore
@@ -3,3 +3,4 @@
/sql_help.c
/psql
+/tmp_check
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index 69bb297fe7..9473ab01cb 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -60,8 +60,15 @@ uninstall:
clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+ rm -rf tmp_check
# files removed here are supposed to be in the distribution tarball,
# so do not clean them in the clean/distclean rules
maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 8254d61099..f6fedb45b7 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -319,7 +319,7 @@ exec_command(const char *cmd,
status = exec_command_ef_ev(scan_state, active_branch, query_buf, true);
else if (strcmp(cmd, "ev") == 0)
status = exec_command_ef_ev(scan_state, active_branch, query_buf, false);
- else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "qecho") == 0)
+ else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "qecho") == 0 || strcmp(cmd, "warn") == 0)
status = exec_command_echo(scan_state, active_branch, cmd);
else if (strcmp(cmd, "elif") == 0)
status = exec_command_elif(scan_state, cstack, query_buf);
@@ -1114,7 +1114,7 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch,
}
/*
- * \echo and \qecho -- echo arguments to stdout or query output
+ * \echo, \qecho, and \warn -- echo arguments to stdout, query output, or stderr
*/
static backslashResult
exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd)
@@ -1129,6 +1129,8 @@ exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd)
if (strcmp(cmd, "qecho") == 0)
fout = pset.queryFout;
+ else if (strcmp(cmd, "warn") == 0)
+ fout = stderr;
else
fout = stdout;
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index d6d41b51d5..581e51246a 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -206,11 +206,12 @@ slashUsage(unsigned short int pager)
fprintf(output, _("Input/Output\n"));
fprintf(output, _(" \\copy ... perform SQL COPY with data stream to the client host\n"));
- fprintf(output, _(" \\echo [STRING] write string to standard output\n"));
+ fprintf(output, _(" \\echo [-n] [STRING] write string to standard output (-n for no newline)\n"));
fprintf(output, _(" \\i FILE execute commands from file\n"));
fprintf(output, _(" \\ir FILE as \\i, but relative to location of current script\n"));
fprintf(output, _(" \\o [FILE] send all query results to file or |pipe\n"));
- fprintf(output, _(" \\qecho [STRING] write string to query output stream (see \\o)\n"));
+ fprintf(output, _(" \\qecho [-n] [STRING] write string to query output stream (-n for no newline, see \\o)\n"));
+ fprintf(output, _(" \\warn [-n] [STRING] write string to standard error (-n for no newline)\n"));
fprintf(output, "\n");
fprintf(output, _("Conditional\n"));
diff --git a/src/bin/psql/t/001_psql.pl b/src/bin/psql/t/001_psql.pl
new file mode 100644
index 0000000000..32dd43279b
--- /dev/null
+++ b/src/bin/psql/t/001_psql.pl
@@ -0,0 +1,32 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More;
+
+my $node = get_new_node('main');
+$node->init();
+$node->start();
+
+# invoke psql
+# - opts: space-separated options and arguments
+# - stat: expected exit status
+# - in: input stream
+# - out: list of re to check on stdout
+# - err: list of re to check on stderr
+# - name: of the test
+sub psql
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+ my ($opts, $stat, $in, $out, $err, $name) = @_;
+ my @cmd = ('psql', split /\s+/, $opts);
+ $node->command_checks_all(\@cmd, $stat, $out, $err, $name, $in);
+ return;
+}
+
+psql('-c \\q', 0, '', [ qr{^$} ], [ qr{^$} ], 'psql -c');
+psql('', 0,"\\echo hello\n\\warn world\n\\q\n", [ qr{^hello$} ], [ qr{^world$} ], 'psql in/out/err');
+
+$node->stop();
+done_testing();
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index bcddc7601e..383a17a7f4 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1421,7 +1421,7 @@ psql_completion(const char *text, int start, int end)
"\\t", "\\T", "\\timing",
"\\unset",
"\\x",
- "\\w", "\\watch",
+ "\\w","\\warn", "\\watch",
"\\z",
"\\!", "\\?",
NULL
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index a164cdbd8c..6ad7f681ae 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -519,22 +519,24 @@ sub command_fails_like
}
# Run a command and check its status and outputs.
-# The 5 arguments are:
+# The 5 to 6 arguments are:
# - cmd: ref to list for command, options and arguments to run
# - ret: expected exit status
# - out: ref to list of re to be checked against stdout (all must match)
# - err: ref to list of re to be checked against stderr (all must match)
# - test_name: name of test
+# - in: standard input
sub command_checks_all
{
local $Test::Builder::Level = $Test::Builder::Level + 1;
- my ($cmd, $expected_ret, $out, $err, $test_name) = @_;
+ my ($cmd, $expected_ret, $out, $err, $test_name, $in) = @_;
+ $in = '' if not defined $in;
# run command
my ($stdout, $stderr);
print("# Running: " . join(" ", @{$cmd}) . "\n");
- IPC::Run::run($cmd, '>', \$stdout, '2>', \$stderr);
+ IPC::Run::run($cmd, '<', \$in, '>', \$stdout, '2>', \$stderr);
# See http://perldoc.perl.org/perlvar.html#%24CHILD_ERROR
my $ret = $?;
--------------2.20.1--
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.
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.
Import Notes
Reply to msg id not found: 10811be6-cd8d-eadb-9b70-7a6e72ed4ee5@gmail.com
(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
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
Attachments:
v7-0001-Add-warn-to-psql.patchtext/x-diff; charset=us-asciiDownload
From 66daae6f73e704ba05dec83b70eebabf9470d768 Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Sun, 21 Apr 2019 11:08:40 -0700
Subject: [PATCH v7] Add \warn to psql
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.20.1"
This is a multi-part message in MIME format.
--------------2.20.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
- Does what it says on the label
- Adds TAP tests for same
- In passing, expose the -n option for \echo, \qecho, and \warn in \? output
create mode 100755 src/bin/psql/t/001_psql.pl
--------------2.20.1
Content-Type: text/x-patch; name="v7-0001-Add-warn-to-psql.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="v7-0001-Add-warn-to-psql.patch"
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index b86764003d..e474efb521 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3220,6 +3220,25 @@ testdb=> <userinput>\setenv LESS -imx4F</userinput>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>\warn <replaceable class="parameter">text</replaceable> [ ... ]</literal></term>
+ <listitem>
+ <para>
+ Prints the arguments to the standard error, separated by one
+ space and followed by a newline. This can be useful to
+ intersperse information in the output of scripts when not polluting
+ standard output is desired. For example:
+
+<programlisting>
+=> <userinput>\warn `date`</userinput>
+Sun Apr 28 21:00:10 PDT 2019
+</programlisting>
+ If the first argument is an unquoted <literal>-n</literal> the trailing
+ newline is not written.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>\watch [ <replaceable class="parameter">seconds</replaceable> ]</literal></term>
diff --git a/src/bin/psql/.gitignore b/src/bin/psql/.gitignore
index c2862b12d6..d324c1c1fa 100644
--- a/src/bin/psql/.gitignore
+++ b/src/bin/psql/.gitignore
@@ -3,3 +3,4 @@
/sql_help.c
/psql
+/tmp_check
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index 69bb297fe7..9473ab01cb 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -60,8 +60,15 @@ uninstall:
clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+ rm -rf tmp_check
# files removed here are supposed to be in the distribution tarball,
# so do not clean them in the clean/distclean rules
maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 8254d61099..f6fedb45b7 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -319,7 +319,7 @@ exec_command(const char *cmd,
status = exec_command_ef_ev(scan_state, active_branch, query_buf, true);
else if (strcmp(cmd, "ev") == 0)
status = exec_command_ef_ev(scan_state, active_branch, query_buf, false);
- else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "qecho") == 0)
+ else if (strcmp(cmd, "echo") == 0 || strcmp(cmd, "qecho") == 0 || strcmp(cmd, "warn") == 0)
status = exec_command_echo(scan_state, active_branch, cmd);
else if (strcmp(cmd, "elif") == 0)
status = exec_command_elif(scan_state, cstack, query_buf);
@@ -1114,7 +1114,7 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch,
}
/*
- * \echo and \qecho -- echo arguments to stdout or query output
+ * \echo, \qecho, and \warn -- echo arguments to stdout, query output, or stderr
*/
static backslashResult
exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd)
@@ -1129,6 +1129,8 @@ exec_command_echo(PsqlScanState scan_state, bool active_branch, const char *cmd)
if (strcmp(cmd, "qecho") == 0)
fout = pset.queryFout;
+ else if (strcmp(cmd, "warn") == 0)
+ fout = stderr;
else
fout = stdout;
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index d6d41b51d5..581e51246a 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -206,11 +206,12 @@ slashUsage(unsigned short int pager)
fprintf(output, _("Input/Output\n"));
fprintf(output, _(" \\copy ... perform SQL COPY with data stream to the client host\n"));
- fprintf(output, _(" \\echo [STRING] write string to standard output\n"));
+ fprintf(output, _(" \\echo [-n] [STRING] write string to standard output (-n for no newline)\n"));
fprintf(output, _(" \\i FILE execute commands from file\n"));
fprintf(output, _(" \\ir FILE as \\i, but relative to location of current script\n"));
fprintf(output, _(" \\o [FILE] send all query results to file or |pipe\n"));
- fprintf(output, _(" \\qecho [STRING] write string to query output stream (see \\o)\n"));
+ fprintf(output, _(" \\qecho [-n] [STRING] write string to query output stream (-n for no newline, see \\o)\n"));
+ fprintf(output, _(" \\warn [-n] [STRING] write string to standard error (-n for no newline)\n"));
fprintf(output, "\n");
fprintf(output, _("Conditional\n"));
diff --git a/src/bin/psql/t/001_psql.pl b/src/bin/psql/t/001_psql.pl
new file mode 100755
index 0000000000..637baa94c9
--- /dev/null
+++ b/src/bin/psql/t/001_psql.pl
@@ -0,0 +1,32 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More;
+
+my $node = get_new_node('main');
+$node->init();
+$node->start();
+
+# invoke psql
+# - opts: space-separated options and arguments
+# - stat: expected exit status
+# - in: input stream
+# - out: list of re to check on stdout
+# - err: list of re to check on stderr
+# - name: of the test
+sub psql
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+ my ($opts, $stat, $in, $out, $err, $name) = @_;
+ my @cmd = ('psql', '-X', split /\s+/, $opts);
+ $node->command_checks_all(\@cmd, $stat, $out, $err, $name, $in);
+ return;
+}
+
+psql('-c \\q', 0, '', [ qr{^$} ], [ qr{^$} ], 'psql -c');
+psql('', 0,"\\echo hello\n\\warn world\n\\q\n", [ qr{^hello$} ], [ qr{^world$} ], 'psql in/out/err');
+
+$node->stop();
+done_testing();
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index bcddc7601e..383a17a7f4 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1421,7 +1421,7 @@ psql_completion(const char *text, int start, int end)
"\\t", "\\T", "\\timing",
"\\unset",
"\\x",
- "\\w", "\\watch",
+ "\\w","\\warn", "\\watch",
"\\z",
"\\!", "\\?",
NULL
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index a164cdbd8c..6ad7f681ae 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -519,22 +519,24 @@ sub command_fails_like
}
# Run a command and check its status and outputs.
-# The 5 arguments are:
+# The 5 to 6 arguments are:
# - cmd: ref to list for command, options and arguments to run
# - ret: expected exit status
# - out: ref to list of re to be checked against stdout (all must match)
# - err: ref to list of re to be checked against stderr (all must match)
# - test_name: name of test
+# - in: standard input
sub command_checks_all
{
local $Test::Builder::Level = $Test::Builder::Level + 1;
- my ($cmd, $expected_ret, $out, $err, $test_name) = @_;
+ my ($cmd, $expected_ret, $out, $err, $test_name, $in) = @_;
+ $in = '' if not defined $in;
# run command
my ($stdout, $stderr);
print("# Running: " . join(" ", @{$cmd}) . "\n");
- IPC::Run::run($cmd, '>', \$stdout, '2>', \$stderr);
+ IPC::Run::run($cmd, '<', \$in, '>', \$stdout, '2>', \$stderr);
# See http://perldoc.perl.org/perlvar.html#%24CHILD_ERROR
my $ret = $?;
--------------2.20.1--
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.
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
Attachments:
easier-warn-test.patchtext/x-diff; charset=us-ascii; name=easier-warn-test.patchDownload
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 4bcf0cc..9b4060f 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -4180,6 +4180,29 @@ drop table psql_serial_tab;
\pset format aligned
\pset expanded off
\pset border 1
+-- \echo and allied features
+\echo this is a test
+this is a test
+\echo -n this is a test without newline
+this is a test without newline\echo more test
+more test
+\set foo bar
+\echo foo = :foo
+foo = bar
+\qecho this is a test
+this is a test
+\qecho -n this is a test without newline
+this is a test without newline\qecho more test
+more test
+\qecho foo = :foo
+foo = bar
+\warn this is a test
+this is a test
+\warn -n this is a test without newline
+this is a test without newline\warn more test
+more test
+\warn foo = :foo
+foo = bar
-- tests for \if ... \endif
\if true
select 'okay';
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 26f436a..2bae6c5 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -771,6 +771,24 @@ drop table psql_serial_tab;
\pset expanded off
\pset border 1
+-- \echo and allied features
+
+\echo this is a test
+\echo -n this is a test without newline
+\echo more test
+\set foo bar
+\echo foo = :foo
+
+\qecho this is a test
+\qecho -n this is a test without newline
+\qecho more test
+\qecho foo = :foo
+
+\warn this is a test
+\warn -n this is a test without newline
+\warn more test
+\warn foo = :foo
+
-- tests for \if ... \endif
\if true
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.
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
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.
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
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
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
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 +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
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 +
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.