psql - factor out echo code

-----Original Message-----
From: Fabien COELHO <coelho@cri.ensmp.fr>
Sent: Sunday, May 30, 2021 6:10 PM
To: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
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

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

Sure. Attached.

--
Fabien.

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

Fabien COELHO <coelho@cri.ensmp.fr> 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

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:

"-- # <whatever> QUERY\n%s\n\n"

with <whatever> in USER DESCRIPTION COMPLETION GEXEC…

--
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> 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:
"-- # <whatever> 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

On 2021-Jul-02, Tom Lane wrote:

Fabien COELHO <coelho@cri.ensmp.fr> 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:
"-- # <whatever> 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

Alvaro Herrera <alvherre@alvh.no-ip.org> 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

"-- # <whatever> 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.

On Sat, Jul 3, 2021 at 3:07 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:

"-- # <whatever> 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

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.

On Sat, Jul 10, 2021 at 10:25 PM Fabien COELHO <coelho@cri.ensmp.fr> 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

Fabien COELHO <coelho@cri.ensmp.fr> 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

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.

Hi

ne 24. 7. 2022 v 21:39 odesílatel Fabien COELHO <coelho@cri.ensmp.fr>
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

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

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.

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.

st 30. 11. 2022 v 10:43 odesílatel Fabien COELHO <coelho@cri.ensmp.fr>
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