Thread: 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.

>-----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.

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.

On 01.12.22 08:27, Pavel Stehule wrote:
> st 30. 11. 2022 v 10:43 odesílatel Fabien COELHO <coelho@cri.ensmp.fr 
> <mailto: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

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.

On Mon, 13 Feb 2023 at 05:41, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> 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