Re: psql - factor out echo code

[Gregory Stark (as CFM)]

On Mon, 13 Feb 2023 at 05:41, Peter Eisentraut
 wrote:
>
> I think this patch requires an up-to-date summary and explanation.  The
> thread is over a year old and the patch has evolved quite a bit.  There
> are some test changes that are not explained.  Please provide more
> detail so that the patch can be considered.

Given this feedback I'm going to mark this Returned with Feedback. I
think it'll be clearer to start with a new thread explaining the
intent of the patch as it is now.

-- 
Gregory Stark
As Commitfest Manager

Re: psql - factor out echo code

[Peter Eisentraut]

On 01.12.22 08:27, Pavel Stehule wrote:
st 30. 11. 2022 v 10:43 odesílatel Fabien COELHO > napsal:



 >> Now some of the output generated by test_extdepend gets a bit
 >> confusing:
 >> +-- QUERY:
 >> +
 >> +
 >> +-- QUERY:
 >>
 >> That's not entirely this patch's fault.  Still that's not really
 >> intuitive to see the output of a query that's just a blank spot..
 >
 > Hmmm.
 >
 > What about adding an explicit \echo before these empty outputs to
mitigate
 > the possible induced confusion?

\echo is not possible.

Attached an attempt to improve the situation by replacing empty
lines with
comments in this test.


I can confirm so all regress tests passed


I think this patch requires an up-to-date summary and explanation.  The 
thread is over a year old and the patch has evolved quite a bit.  There 
are some test changes that are not explained.  Please provide more 
detail so that the patch can be considered.

Re: psql - factor out echo code

[Pavel Stehule]

st 30. 11. 2022 v 10:43 odesílatel Fabien COELHO 
napsal:

>
> >> Now some of the output generated by test_extdepend gets a bit
> >> confusing:
> >> +-- QUERY:
> >> +
> >> +
> >> +-- QUERY:
> >>
> >> That's not entirely this patch's fault.  Still that's not really
> >> intuitive to see the output of a query that's just a blank spot..
> >
> > Hmmm.
> >
> > What about adding an explicit \echo before these empty outputs to
> mitigate
> > the possible induced confusion?
>
> \echo is not possible.
>
> Attached an attempt to improve the situation by replacing empty lines with
> comments in this test.
>

I can confirm so all regress tests passed

Regards

Pavel


>
> --
> Fabien.

Re: psql - factor out echo code

[Fabien COELHO]

Now some of the output generated by test_extdepend gets a bit
confusing:
+-- QUERY:
+
+
+-- QUERY:

That's not entirely this patch's fault.  Still that's not really
intuitive to see the output of a query that's just a blank spot..


Hmmm.

What about adding an explicit \echo before these empty outputs to mitigate 
the possible induced confusion?


\echo is not possible.

Attached an attempt to improve the situation by replacing empty lines with 
comments in this test.


--
Fabien.diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 7672ed9e9d..16873aff62 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5284,18 +5284,9 @@ echo_hidden_command(const char *query)
 {
 	if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
 	{
-		printf(_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
-		fflush(stdout);
+		echoQuery(stdout, true, query);
 		if (pset.logfile)
-		{
-			fprintf(pset.logfile,
-	_("* QUERY **\n"
-	  "%s\n"
-	  "**\n\n"), query);
-			fflush(pset.logfile);
-		}
+			echoQuery(pset.logfile, true, query);
 
 		if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
 			return false;
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index b989d792aa..30fd5bcda1 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -543,6 +543,18 @@ PrintTiming(double elapsed_msec)
 		   elapsed_msec, days, (int) hours, (int) minutes, seconds);
 }
 
+/*
+ * Echo user query
+ */
+void
+echoQuery(FILE *out, bool is_internal, const char *query)
+{
+	if (is_internal)
+		fprintf(out, _("-- INTERNAL QUERY:\n%s\n\n"), query);
+	else
+		fprintf(out, _("-- QUERY:\n%s\n\n"), query);
+	fflush(out);
+}
 
 /*
  * PSQLexec
@@ -569,18 +581,9 @@ PSQLexec(const char *query)
 
 	if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
 	{
-		printf(_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
-		fflush(stdout);
+		echoQuery(stdout, true, query);
 		if (pset.logfile)
-		{
-			fprintf(pset.logfile,
-	_("* QUERY **\n"
-	  "%s\n"
-	  "**\n\n"), query);
-			fflush(pset.logfile);
-		}
+			echoQuery(pset.logfile, true, query);
 
 		if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
 			return NULL;
@@ -796,10 +799,7 @@ ExecQueryTuples(const PGresult *result)
  * assumes that MainLoop did that, so we have to do it here.
  */
 if (pset.echo == PSQL_ECHO_ALL && !pset.singlestep)
-{
-	puts(query);
-	fflush(stdout);
-}
+	echoQuery(stdout, false, query);
 
 if (!SendQuery(query))
 {
@@ -1058,13 +1058,7 @@ SendQuery(const char *query)
 	}
 
 	if (pset.logfile)
-	{
-		fprintf(pset.logfile,
-_("* QUERY **\n"
-  "%s\n"
-  "**\n\n"), query);
-		fflush(pset.logfile);
-	}
+		echoQuery(pset.logfile, false, query);
 
 	SetCancelConn(pset.db);
 
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index f0820dd7d5..359c48143e 100644
--- a/src/bin/psql/common.h
+++ b/src/bin/psql/common.h
@@ -32,6 +32,7 @@ extern void psql_setup_cancel_handler(void);
 extern PGresult *PSQLexec(const char *query);
 extern int	PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE *printQueryFout);
 
+extern void echoQuery(FILE *out, bool is_internal, const char *query);
 extern bool SendQuery(const char *query);
 
 extern bool is_superuser(void);
diff --git a/src/test/modules/test_extensions/expected/test_extdepend.out b/src/test/modules/test_extensions/expected/test_extdepend.out
index 0b62015d18..a08ef8d19d 100644
--- a/src/test/modules/test_extensions/expected/test_extdepend.out
+++ b/src/test/modules/test_extensions/expected/test_extdepend.out
@@ -11,17 +11,17 @@ SELECT * FROM test_extdep_commands;
   CREATE EXTENSION test_ext5 SCHEMA test_ext
   SET search_path TO test_ext
   CREATE TABLE a (a1 int)
- 
+  -- function b
   CREATE FUNCTION b() RETURNS TRIGGER LANGUAGE plpgsql AS   +
 $$ BEGIN NEW.a1 := NEW.a1 + 42; RETURN NEW; END; $$
   ALTER FUNCTION b() DEPENDS ON EXTENSION test_ext5
- 
+  -- trigger c
   CREATE TRIGGER c BEFORE INSERT ON a FOR EACH ROW EXECUTE PROCEDURE b()
   ALTER TRIGGER c ON a DEPENDS ON EXTENSION test_ext5
- 
+  -- materialized view d
   CREATE MATERIALIZED VIEW d AS SELECT * FROM a
   ALTER MATERIALIZED VIEW d DEPENDS ON EXTENSION test_ext5
- 
+  -- index e
   CREATE INDEX e ON a (a1)
   ALTER INDEX e DEPENDS ON EXTENSION test_ext5
   RESET search_path
@@ -29,24 +29,58 @@ SELECT * FROM test_extdep_commands;
 
 -- First, test that dependent objects go away when the extension is dropped.
 SELECT * FROM test_extdep_commands \gexec
+-- QUERY:
  CREATE SCHEMA test_ext
+
+-- QUERY:
  CREATE EXTENSION test_ext5 SCHEMA test_ext
+
+-- QUERY:
  SET search_path TO test_ext
+
+-- QUERY:
  CREATE TABLE a (a1 int)
 
+-- QUERY:
+ -- function b
+
+-- QUERY:
  CREATE FUNCTION b() RETURNS TRIGGER LANGUAGE plpgsql AS
$$ 

Re: psql - factor out echo code

[Fabien COELHO]

Hmm.  The refactoring is worth it as much as the differentiation
between QUERY and INTERNAL QUERY as the same pattern is repeated 5
times.

Now some of the output generated by test_extdepend gets a bit
confusing:
+-- QUERY:
+
+
+-- QUERY:

That's not entirely this patch's fault.  Still that's not really
intuitive to see the output of a query that's just a blank spot..


Hmmm.

What about adding an explicit \echo before these empty outputs to mitigate 
the possible induced confusion?


--
Fabien.

Re: psql - factor out echo code

[Michael Paquier]

On Sun, Jul 24, 2022 at 10:23:39PM +0200, Pavel Stehule wrote:
> I had just one question - with this patch, the format of output of modes
> ECHO ALL and ECHO QUERIES will be different, and that can be a little bit
> messy. On second hand, the prefix --QUERY can be disturbing in echo queries
> mode. It is not a problem in echo all mode, because queries and results are
> mixed together. So in the end, I think the current design can work.
> 
> All tests passed, this is trivial patch without impacts on users
> 
> I'll mark this patch as ready for committer

Hmm.  The refactoring is worth it as much as the differentiation
between QUERY and INTERNAL QUERY as the same pattern is repeated 5
times.

Now some of the output generated by test_extdepend gets a bit
confusing:
+-- QUERY:
+
+
+-- QUERY:

That's not entirely this patch's fault.  Still that's not really
intuitive to see the output of a query that's just a blank spot..
--
Michael


signature.asc
Description: PGP signature

Re: psql - factor out echo code

[Pavel Stehule]

Hi


ne 24. 7. 2022 v 21:39 odesílatel Fabien COELHO 
napsal:

>
> >> Attached v4 simplifies the format and fixes this one.
> >
> > I think this goes way way overboard in terms of invasiveness. There's no
> > need to identify individual call sites of PSQLexec. [...]
>
> ISTM that having the information was useful for the user who actually
> asked for psql to show hidden queries, and pretty simple to get, although
> somehow invasive.
>
> > It also looks like a mess from the translatibility standpoint.
> > You can't expect "%s QUERY" to be a useful thing for translators.
>
> Sure. Maybe I should have used an enum have a explicit switch in
> echoQuery, but I do not like writing this kind of code.
>
> Attached a v5 without hinting at the origin of the query beyond internal
> or not.
>


I had just one question - with this patch, the format of output of modes
ECHO ALL and ECHO QUERIES will be different, and that can be a little bit
messy. On second hand, the prefix --QUERY can be disturbing in echo queries
mode. It is not a problem in echo all mode, because queries and results are
mixed together. So in the end, I think the current design can work.

All tests passed, this is trivial patch without impacts on users

I'll mark this patch as ready for committer

Regards

Pavel

Re: psql - factor out echo code

[Fabien COELHO]

Attached v4 simplifies the format and fixes this one.


I think this goes way way overboard in terms of invasiveness. There's no 
need to identify individual call sites of PSQLexec. [...]


ISTM that having the information was useful for the user who actually 
asked for psql to show hidden queries, and pretty simple to get, although 
somehow invasive.



It also looks like a mess from the translatibility standpoint.
You can't expect "%s QUERY" to be a useful thing for translators.


Sure. Maybe I should have used an enum have a explicit switch in 
echoQuery, but I do not like writing this kind of code.


Attached a v5 without hinting at the origin of the query beyond internal 
or not.


--
Fabien.diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index d704c4220c..7e91e588cc 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5112,18 +5112,9 @@ echo_hidden_command(const char *query)
 {
 	if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
 	{
-		printf(_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
-		fflush(stdout);
+		echoQuery(stdout, true, query);
 		if (pset.logfile)
-		{
-			fprintf(pset.logfile,
-	_("* QUERY **\n"
-	  "%s\n"
-	  "**\n\n"), query);
-			fflush(pset.logfile);
-		}
+			echoQuery(pset.logfile, true, query);
 
 		if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
 			return false;
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 5640786678..15dab3c543 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -523,6 +523,18 @@ PrintTiming(double elapsed_msec)
 		   elapsed_msec, days, (int) hours, (int) minutes, seconds);
 }
 
+/*
+ * Echo user query
+ */
+void
+echoQuery(FILE *out, bool is_internal, const char *query)
+{
+	if (is_internal)
+		fprintf(out, _("-- INTERNAL QUERY:\n%s\n\n"), query);
+	else
+		fprintf(out, _("-- QUERY:\n%s\n\n"), query);
+	fflush(out);
+}
 
 /*
  * PSQLexec
@@ -549,18 +561,9 @@ PSQLexec(const char *query)
 
 	if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
 	{
-		printf(_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
-		fflush(stdout);
+		echoQuery(stdout, true, query);
 		if (pset.logfile)
-		{
-			fprintf(pset.logfile,
-	_("* QUERY **\n"
-	  "%s\n"
-	  "**\n\n"), query);
-			fflush(pset.logfile);
-		}
+			echoQuery(pset.logfile, true, query);
 
 		if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
 			return NULL;
@@ -859,10 +862,7 @@ ExecQueryTuples(const PGresult *result)
  * assumes that MainLoop did that, so we have to do it here.
  */
 if (pset.echo == PSQL_ECHO_ALL && !pset.singlestep)
-{
-	puts(query);
-	fflush(stdout);
-}
+	echoQuery(stdout, false, query);
 
 if (!SendQuery(query))
 {
@@ -1229,13 +1229,7 @@ SendQuery(const char *query)
 	}
 
 	if (pset.logfile)
-	{
-		fprintf(pset.logfile,
-_("* QUERY **\n"
-  "%s\n"
-  "**\n\n"), query);
-		fflush(pset.logfile);
-	}
+		echoQuery(pset.logfile, false, query);
 
 	SetCancelConn(pset.db);
 
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index d8538a4e06..04ece4e9b1 100644
--- a/src/bin/psql/common.h
+++ b/src/bin/psql/common.h
@@ -31,6 +31,7 @@ extern void psql_setup_cancel_handler(void);
 extern PGresult *PSQLexec(const char *query);
 extern int	PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE *printQueryFout);
 
+extern void echoQuery(FILE *out, bool is_internal, const char *query);
 extern bool SendQuery(const char *query);
 
 extern bool is_superuser(void);
diff --git a/src/test/modules/test_extensions/expected/test_extdepend.out b/src/test/modules/test_extensions/expected/test_extdepend.out
index 0b62015d18..5fd826ac56 100644
--- a/src/test/modules/test_extensions/expected/test_extdepend.out
+++ b/src/test/modules/test_extensions/expected/test_extdepend.out
@@ -29,24 +29,58 @@ SELECT * FROM test_extdep_commands;
 
 -- First, test that dependent objects go away when the extension is dropped.
 SELECT * FROM test_extdep_commands \gexec
+-- QUERY:
  CREATE SCHEMA test_ext
+
+-- QUERY:
  CREATE EXTENSION test_ext5 SCHEMA test_ext
+
+-- QUERY:
  SET search_path TO test_ext
+
+-- QUERY:
  CREATE TABLE a (a1 int)
 
+-- QUERY:
+
+
+-- QUERY:
  CREATE FUNCTION b() RETURNS TRIGGER LANGUAGE plpgsql AS
$$ BEGIN NEW.a1 := NEW.a1 + 42; RETURN NEW; END; $$
+
+-- QUERY:
  ALTER FUNCTION b() DEPENDS ON EXTENSION test_ext5
 
+-- QUERY:
+
+
+-- QUERY:
  CREATE TRIGGER c BEFORE INSERT ON a FOR EACH ROW EXECUTE PROCEDURE b()
+
+-- QUERY:
  ALTER TRIGGER c ON a DEPENDS ON EXTENSION test_ext5
 
+-- QUERY:
+
+
+-- QUERY:
  CREATE MATERIALIZED VIEW d AS SELECT * FROM a
+
+-- QUERY:
  ALTER MATERIALIZED VIEW d DEPENDS ON EXTENSION test_ext5
 
+-- QUERY:
+
+
+-- QUERY:
  CREATE INDEX e ON a (a1)
+
+-- QUERY:
  ALTER INDEX e DEPENDS ON EXTENSION test_ext5
+
+-- QUERY:
 

Re: psql - factor out echo code

[Tom Lane]

Fabien COELHO  writes:
> Attached v4 simplifies the format and fixes this one.

I think this goes way way overboard in terms of invasiveness.
There's no need to identify individual call sites of PSQLexec.
We didn't have anything like that level of detail before, and
there has been no field demand for it either.  What I had
in mind was basically to identify the call sites of echoQuery,
ie distinguish user commands from psql-generated commands
with labels like "QUERY:" vs "INTERNAL QUERY:".  We don't
need to change the APIs of existing functions, I don't think.

It also looks like a mess from the translatibility standpoint.
You can't expect "%s QUERY" to be a useful thing for translators.

regards, tom lane

Re: psql - factor out echo code

[vignesh C]

On Sat, Jul 10, 2021 at 10:25 PM Fabien COELHO  wrote:
>
>
> Hello Vignesh,
>
> > I am changing the status to "Needs review" as the review is not
> > completed for this patch and also there are some tests failing, that
> > need to be fixed:
> > test test_extdepend   ... FAILED   50 ms
>
> Indeed,
>
> Attached v4 simplifies the format and fixes this one.
> I ran check-world, this time.

Thanks for posting an updated patch, the tests are passing now. I have
changed the status back to Ready For Committer.

Regards,
Vignesh

Re: psql - factor out echo code

[Fabien COELHO]

Hello Vignesh,


I am changing the status to "Needs review" as the review is not
completed for this patch and also there are some tests failing, that
need to be fixed:
test test_extdepend   ... FAILED   50 ms


Indeed,

Attached v4 simplifies the format and fixes this one.
I ran check-world, this time.

--
Fabien.diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 543401c6d6..8ec00881db 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2061,7 +2061,7 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
 printfPQExpBuffer(, "ALTER USER %s PASSWORD ",
   fmtId(user));
 appendStringLiteralConn(, encrypted_password, pset.db);
-res = PSQLexec(buf.data);
+res = PSQLexec("PASSWORD", buf.data);
 termPQExpBuffer();
 if (!res)
 	success = false;
@@ -4993,22 +4993,13 @@ do_watch(PQExpBuffer query_buf, double sleep)
  * returns true unless we have ECHO_HIDDEN_NOEXEC.
  */
 static bool
-echo_hidden_command(const char *query)
+echo_hidden_command(const char *origin, const char *query)
 {
 	if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
 	{
-		printf(_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
-		fflush(stdout);
+		echoQuery(stdout, origin, query);
 		if (pset.logfile)
-		{
-			fprintf(pset.logfile,
-	_("* QUERY **\n"
-	  "%s\n"
-	  "**\n\n"), query);
-			fflush(pset.logfile);
-		}
+			echoQuery(pset.logfile, origin, query);
 
 		if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
 			return false;
@@ -5060,7 +5051,7 @@ lookup_object_oid(EditableObjectType obj_type, const char *desc,
 			break;
 	}
 
-	if (!echo_hidden_command(query->data))
+	if (!echo_hidden_command("INTERNAL", query->data))
 	{
 		destroyPQExpBuffer(query);
 		return false;
@@ -5155,7 +5146,7 @@ get_create_object_cmd(EditableObjectType obj_type, Oid oid,
 			break;
 	}
 
-	if (!echo_hidden_command(query->data))
+	if (!echo_hidden_command("INTERNAL", query->data))
 	{
 		destroyPQExpBuffer(query);
 		return false;
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 9a00499510..50e8023c90 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -523,6 +523,17 @@ PrintTiming(double elapsed_msec)
 		   elapsed_msec, days, (int) hours, (int) minutes, seconds);
 }
 
+/*
+ * Echo user query
+ */
+void
+echoQuery(FILE *out, const char *origin, const char *query)
+{
+	fprintf(out,
+			/* FIXME should this really be translatable? */
+			_("-- %s QUERY:\n%s\n\n"), origin, query);
+	fflush(out);
+}
 
 /*
  * PSQLexec
@@ -537,7 +548,7 @@ PrintTiming(double elapsed_msec)
  * caller uses this path to issue "SET CLIENT_ENCODING".
  */
 PGresult *
-PSQLexec(const char *query)
+PSQLexec(const char *origin, const char *query)
 {
 	PGresult   *res;
 
@@ -549,18 +560,9 @@ PSQLexec(const char *query)
 
 	if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
 	{
-		printf(_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
-		fflush(stdout);
+		echoQuery(stdout, origin, query);
 		if (pset.logfile)
-		{
-			fprintf(pset.logfile,
-	_("* QUERY **\n"
-	  "%s\n"
-	  "**\n\n"), query);
-			fflush(pset.logfile);
-		}
+			echoQuery(pset.logfile, origin, query);
 
 		if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
 			return NULL;
@@ -856,10 +858,7 @@ ExecQueryTuples(const PGresult *result)
  * assumes that MainLoop did that, so we have to do it here.
  */
 if (pset.echo == PSQL_ECHO_ALL && !pset.singlestep)
-{
-	puts(query);
-	fflush(stdout);
-}
+	echoQuery(stdout, "GEXEC", query);
 
 if (!SendQuery(query))
 {
@@ -1226,13 +1225,7 @@ SendQuery(const char *query)
 	}
 
 	if (pset.logfile)
-	{
-		fprintf(pset.logfile,
-_("* QUERY **\n"
-  "%s\n"
-  "**\n\n"), query);
-		fflush(pset.logfile);
-	}
+		echoQuery(pset.logfile, "USER", query);
 
 	SetCancelConn(pset.db);
 
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index 041b2ac068..cdad55414a 100644
--- a/src/bin/psql/common.h
+++ b/src/bin/psql/common.h
@@ -28,9 +28,10 @@ extern sigjmp_buf sigint_interrupt_jmp;
 
 extern void psql_setup_cancel_handler(void);
 
-extern PGresult *PSQLexec(const char *query);
+extern PGresult *PSQLexec(const char *origin, const char *query);
 extern int	PSQLexecWatch(const char *query, const printQueryOpt *opt);
 
+extern void echoQuery(FILE *out, const char *origin, const char *query);
 extern bool SendQuery(const char *query);
 
 extern bool is_superuser(void);
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 2abf255798..b7f701a68a 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -127,7 +127,7 @@ describeAggregates(const char *pattern, bool verbose, bool showSystem)
 
 	appendPQExpBufferStr(, "ORDER BY 1, 2, 4;");
 
-	res = PSQLexec(buf.data);
+	res 

Re: psql - factor out echo code

[vignesh C]

On Sat, Jul 3, 2021 at 3:07 AM Fabien COELHO  wrote:
>
>
> >  "-- #  QUERY\n%s\n\n"
>
> Attached an attempt along those lines. I found another duplicate of the
> ascii-art printing in another function.
>
> Completion queries seems to be out of the echo/echo hidden feature.
>
> Incredible, there is a (small) impact on regression tests for the \gexec
> case. All other changes have no impact, because they are not tested:-(

I am changing the status to "Needs review" as the review is not
completed for this patch and also there are some tests failing, that
need to be fixed:
test test_extdepend   ... FAILED   50 ms

Regards,
Vignesh

Re: psql - factor out echo code

[Fabien COELHO]

 "-- #  QUERY\n%s\n\n"


Attached an attempt along those lines. I found another duplicate of the 
ascii-art printing in another function.


Completion queries seems to be out of the echo/echo hidden feature.

Incredible, there is a (small) impact on regression tests for the \gexec 
case. All other changes have no impact, because they are not tested:-(


--
Fabien.diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 543401c6d6..8ec00881db 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2061,7 +2061,7 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
 printfPQExpBuffer(, "ALTER USER %s PASSWORD ",
   fmtId(user));
 appendStringLiteralConn(, encrypted_password, pset.db);
-res = PSQLexec(buf.data);
+res = PSQLexec("PASSWORD", buf.data);
 termPQExpBuffer();
 if (!res)
 	success = false;
@@ -4993,22 +4993,13 @@ do_watch(PQExpBuffer query_buf, double sleep)
  * returns true unless we have ECHO_HIDDEN_NOEXEC.
  */
 static bool
-echo_hidden_command(const char *query)
+echo_hidden_command(const char *origin, const char *query)
 {
 	if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
 	{
-		printf(_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
-		fflush(stdout);
+		echoQuery(stdout, origin, query);
 		if (pset.logfile)
-		{
-			fprintf(pset.logfile,
-	_("* QUERY **\n"
-	  "%s\n"
-	  "**\n\n"), query);
-			fflush(pset.logfile);
-		}
+			echoQuery(pset.logfile, origin, query);
 
 		if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
 			return false;
@@ -5060,7 +5051,7 @@ lookup_object_oid(EditableObjectType obj_type, const char *desc,
 			break;
 	}
 
-	if (!echo_hidden_command(query->data))
+	if (!echo_hidden_command("INTERNAL", query->data))
 	{
 		destroyPQExpBuffer(query);
 		return false;
@@ -5155,7 +5146,7 @@ get_create_object_cmd(EditableObjectType obj_type, Oid oid,
 			break;
 	}
 
-	if (!echo_hidden_command(query->data))
+	if (!echo_hidden_command("INTERNAL", query->data))
 	{
 		destroyPQExpBuffer(query);
 		return false;
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 9a00499510..69d4e33093 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -523,6 +523,17 @@ PrintTiming(double elapsed_msec)
 		   elapsed_msec, days, (int) hours, (int) minutes, seconds);
 }
 
+/*
+ * Echo user query
+ */
+void
+echoQuery(FILE *out, const char *origin, const char *query)
+{
+	fprintf(out,
+			/* FIXME should this really be translatable? */
+			_("-- # %s QUERY\n%s\n\n"), origin, query);
+	fflush(out);
+}
 
 /*
  * PSQLexec
@@ -537,7 +548,7 @@ PrintTiming(double elapsed_msec)
  * caller uses this path to issue "SET CLIENT_ENCODING".
  */
 PGresult *
-PSQLexec(const char *query)
+PSQLexec(const char *origin, const char *query)
 {
 	PGresult   *res;
 
@@ -549,18 +560,9 @@ PSQLexec(const char *query)
 
 	if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
 	{
-		printf(_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
-		fflush(stdout);
+		echoQuery(stdout, origin, query);
 		if (pset.logfile)
-		{
-			fprintf(pset.logfile,
-	_("* QUERY **\n"
-	  "%s\n"
-	  "**\n\n"), query);
-			fflush(pset.logfile);
-		}
+			echoQuery(pset.logfile, origin, query);
 
 		if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
 			return NULL;
@@ -856,10 +858,7 @@ ExecQueryTuples(const PGresult *result)
  * assumes that MainLoop did that, so we have to do it here.
  */
 if (pset.echo == PSQL_ECHO_ALL && !pset.singlestep)
-{
-	puts(query);
-	fflush(stdout);
-}
+	echoQuery(stdout, "GEXEC", query);
 
 if (!SendQuery(query))
 {
@@ -1226,13 +1225,7 @@ SendQuery(const char *query)
 	}
 
 	if (pset.logfile)
-	{
-		fprintf(pset.logfile,
-_("* QUERY **\n"
-  "%s\n"
-  "**\n\n"), query);
-		fflush(pset.logfile);
-	}
+		echoQuery(pset.logfile, "USER", query);
 
 	SetCancelConn(pset.db);
 
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index 041b2ac068..cdad55414a 100644
--- a/src/bin/psql/common.h
+++ b/src/bin/psql/common.h
@@ -28,9 +28,10 @@ extern sigjmp_buf sigint_interrupt_jmp;
 
 extern void psql_setup_cancel_handler(void);
 
-extern PGresult *PSQLexec(const char *query);
+extern PGresult *PSQLexec(const char *origin, const char *query);
 extern int	PSQLexecWatch(const char *query, const printQueryOpt *opt);
 
+extern void echoQuery(FILE *out, const char *origin, const char *query);
 extern bool SendQuery(const char *query);
 
 extern bool is_superuser(void);
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 2abf255798..b7f701a68a 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -127,7 +127,7 @@ describeAggregates(const char *pattern, bool verbose, bool showSystem)
 
 	appendPQExpBufferStr(, "ORDER BY 1, 2, 4;");

Re: psql - factor out echo code

[Tom Lane]

Alvaro Herrera  writes:
> I think the most interesting case for decoration is the "step by step"
> mode, where you want the "title" that precedes each query be easily
> visible.

I'm okay with leaving the step-by-step prompt as-is, personally.
It's the inconsistency of the other ones that bugs me.

> Also: one place that prints queries that wasn't mentioned before is
> exec_command_print() in command.c.

Ah, I was wondering if anyplace outside common.c did so.  But that
one seems to me to be a different animal -- it's not logging
queries-about-to-be-executed.

regards, tom lane

Re: psql - factor out echo code

[Alvaro Herrera]

On 2021-Jul-02, Tom Lane wrote:

> Fabien COELHO  writes:
> > Yes. Maybe decorations should be SQL comments, and the purpose/origin of 
> > the query could be made clear as you suggest, eg something like markdown 
> > in a comment:
> >"-- #  QUERY\n%s\n\n"
> 
> If we keep the decoration, I'd agree with dropping all the asterisks.
> I'd vote for something pretty minimalistic, like
> 
>   -- INTERNAL QUERY:

I think the most interesting case for decoration is the "step by step"
mode, where you want the "title" that precedes each query be easily
visible.  I think two uppercase words are not sufficient for that ...
and Markdown format which would force you to convert to HTML before you
can notice where it is, are not sufficient either.  The line with a few
asterisks seems fine to me for that.  Removing the asterisks in the
other case seems fine.  I admit I don't use the step-by-step mode all
that much, though.

Also: one place that prints queries that wasn't mentioned before is
exec_command_print() in command.c.

-- 
Álvaro Herrera  Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Ed is the standard text editor."
  http://groups.google.com/group/alt.religion.emacs/msg/8d94ddab6a9b0ad3

Re: psql - factor out echo code

[Tom Lane]

Fabien COELHO  writes:
> Yes. Maybe decorations should be SQL comments, and the purpose/origin of 
> the query could be made clear as you suggest, eg something like markdown 
> in a comment:
>"-- #  QUERY\n%s\n\n"

If we keep the decoration, I'd agree with dropping all the asterisks.
I'd vote for something pretty minimalistic, like

-- INTERNAL QUERY:

regards, tom lane

Re: psql - factor out echo code

[Fabien COELHO]

Hello Tom,


I went to commit this, figuring that it was a trivial bit of code
consolidation, but as I looked around in common.c I got rather
unhappy with the inconsistent behavior of things.  Examining
the various places that implement "echo"-related logic, we have
the three places this patch proposes to unify, which log queries
using

   fprintf(out,
   _("* QUERY **\n"
 "%s\n"
 "**\n\n"), query);

and then we have two more that just do

   puts(query);

plus this:

   if (!OK && pset.echo == PSQL_ECHO_ERRORS)
   pg_log_info("STATEMENT:  %s", query);

So it's exactly fifty-fifty as to whether we add all that decoration
or none at all.  I think if we're going to touch this logic, we
ought to try to unify the behavior.


+1.

I did not go this way because I wanted it to be a simple restructuring 
patch so that it could go through without much ado, but I agree with 
improving the current status. I'm not sure we want too much ascii-art.


My vote would be to drop the decoration everywhere, but perhaps there 
are votes not to?


No, I'd be ok with removing the decoration, or at least simplify them, or 
as you suggest below make the have a useful semantics.



A different angle is that the identical decoration is used for both
psql-generated queries that are logged because of ECHO_HIDDEN, and
user-entered queries.  This seems at best rather unhelpful.


Indeed.

If we keep the decoration, should we make it different for those two 
cases?  (Maybe "INTERNAL QUERY" vs "QUERY", for example.)  The cases 
with no decoration likewise fall into multiple categories, both 
user-entered and generated-by-gexec; if we were going with a decorated 
approach I'd think it useful to make a distinction between those, too.


Thoughts?


Yes. Maybe decorations should be SQL comments, and the purpose/origin of 
the query could be made clear as you suggest, eg something like markdown 
in a comment:


  "-- #  QUERY\n%s\n\n"

with  in USER DESCRIPTION COMPLETION GEXEC…

--
Fabien.

Re: psql - factor out echo code

[Tom Lane]

Fabien COELHO  writes:
> [ psql-echo-2.patch ]

I went to commit this, figuring that it was a trivial bit of code
consolidation, but as I looked around in common.c I got rather
unhappy with the inconsistent behavior of things.  Examining
the various places that implement "echo"-related logic, we have
the three places this patch proposes to unify, which log queries
using

fprintf(out,
_("* QUERY **\n"
  "%s\n"
  "**\n\n"), query);

and then we have two more that just do

puts(query);

plus this:

if (!OK && pset.echo == PSQL_ECHO_ERRORS)
pg_log_info("STATEMENT:  %s", query);

So it's exactly fifty-fifty as to whether we add all that decoration
or none at all.  I think if we're going to touch this logic, we
ought to try to unify the behavior.  My vote would be to drop the
decoration everywhere, but perhaps there are votes not to?

A different angle is that the identical decoration is used for both
psql-generated queries that are logged because of ECHO_HIDDEN, and
user-entered queries.  This seems at best rather unhelpful.  If
we keep the decoration, should we make it different for those two
cases?  (Maybe "INTERNAL QUERY" vs "QUERY", for example.)  The
cases with no decoration likewise fall into multiple categories,
both user-entered and generated-by-gexec; if we were going with
a decorated approach I'd think it useful to make a distinction
between those, too.

Thoughts?

regards, tom lane

RE: psql - factor out echo code

[Shinya11.Kato]

>> Wouldn't it be better to comment it like any other function?
>
>Sure. Attached.

Thank you for your revision.
I think this patch is good, so I will move it to ready for committer.

Best regards,
Shinya Kato

RE: psql - factor out echo code

[Fabien COELHO]

Wouldn't it be better to comment it like any other function?


Sure. Attached.

--
Fabien.diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 9a00499510..00e5bf290b 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -523,6 +523,18 @@ PrintTiming(double elapsed_msec)
 		   elapsed_msec, days, (int) hours, (int) minutes, seconds);
 }
 
+/*
+ * Echo user query
+ */
+static void
+echoQuery(FILE *out, const char *query)
+{
+	fprintf(out,
+			_("* QUERY **\n"
+			  "%s\n"
+			  "**\n\n"), query);
+	fflush(out);
+}
 
 /*
  * PSQLexec
@@ -549,18 +561,9 @@ PSQLexec(const char *query)
 
 	if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
 	{
-		printf(_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
-		fflush(stdout);
+		echoQuery(stdout, query);
 		if (pset.logfile)
-		{
-			fprintf(pset.logfile,
-	_("* QUERY **\n"
-	  "%s\n"
-	  "**\n\n"), query);
-			fflush(pset.logfile);
-		}
+			echoQuery(pset.logfile, query);
 
 		if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
 			return NULL;
@@ -1226,13 +1229,7 @@ SendQuery(const char *query)
 	}
 
 	if (pset.logfile)
-	{
-		fprintf(pset.logfile,
-_("* QUERY **\n"
-  "%s\n"
-  "**\n\n"), query);
-		fflush(pset.logfile);
-	}
+		echoQuery(pset.logfile, query);
 
 	SetCancelConn(pset.db);
 

RE: psql - factor out echo code

[Shinya11.Kato]

>-Original Message-
>From: Fabien COELHO 
>Sent: Sunday, May 30, 2021 6:10 PM
>To: PostgreSQL Developers 
>Subject: psql - factor out echo code
>
>
>While working on something in "psql/common.c" I noticed some triplicated code,
>including a long translatable string. This minor patch refactors this in one
>function.
>
>--
>Fabien.

Wouldn't it be better to comment it like any other function?

Best regards,
Shinya Kato

psql - factor out echo code

[Fabien COELHO]

While working on something in "psql/common.c" I noticed some triplicated 
code, including a long translatable string. This minor patch refactors 
this in one function.


--
Fabien.diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 7a95465111..4fd80ec6bb 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -523,6 +523,15 @@ PrintTiming(double elapsed_msec)
 		   elapsed_msec, days, (int) hours, (int) minutes, seconds);
 }
 
+static void
+echoQuery(FILE *out, const char *query)
+{
+	fprintf(out,
+			_("* QUERY **\n"
+			  "%s\n"
+			  "**\n\n"), query);
+	fflush(out);
+}
 
 /*
  * PSQLexec
@@ -549,18 +558,9 @@ PSQLexec(const char *query)
 
 	if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
 	{
-		printf(_("* QUERY **\n"
- "%s\n"
- "**\n\n"), query);
-		fflush(stdout);
+		echoQuery(stdout, query);
 		if (pset.logfile)
-		{
-			fprintf(pset.logfile,
-	_("* QUERY **\n"
-	  "%s\n"
-	  "**\n\n"), query);
-			fflush(pset.logfile);
-		}
+			echoQuery(pset.logfile, query);
 
 		if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
 			return NULL;
@@ -1226,13 +1226,7 @@ SendQuery(const char *query)
 	}
 
 	if (pset.logfile)
-	{
-		fprintf(pset.logfile,
-_("* QUERY **\n"
-  "%s\n"
-  "**\n\n"), query);
-		fflush(pset.logfile);
-	}
+		echoQuery(pset.logfile, query);
 
 	SetCancelConn(pset.db);