Re: Adding comments to help understand psql hidden queries
On Thu, Apr 4, 2024 at 11:12 AM David Christensen wrote:
>
> On Thu, Apr 4, 2024 at 9:32 AM Peter Eisentraut wrote:
> >
> > On 03.04.24 19:16, David Christensen wrote:
> > > I removed _() in the output of the query/stars since there'd be no
> > > sensible existing translations for the constructed string, which
> > > included the query string itself. If we need it for the "QUERY"
> > > string, this could be added fairly easily, but the existing piece
> > > would have been nonsensical and never used in practice.
> >
> > "QUERY" is currently translated. Your patch loses that.
>
> I see; enclosed is v5 which fixes this.
>
> The effective diff from the last one is:
>
> -char *label = "QUERY";
> +char *label = _("QUERY");
>
> and
>
> -label = psprintf("QUERY (\\%s)", curcmd);
> +label = psprintf(_("QUERY (\\%s)"), curcmd);
Any further concerns/issues with this patch that I can address to help
move it forward?
David
Re: Adding comments to help understand psql hidden queries
On Thu, Apr 4, 2024 at 9:32 AM Peter Eisentraut wrote:
>
> On 03.04.24 19:16, David Christensen wrote:
> > I removed _() in the output of the query/stars since there'd be no
> > sensible existing translations for the constructed string, which
> > included the query string itself. If we need it for the "QUERY"
> > string, this could be added fairly easily, but the existing piece
> > would have been nonsensical and never used in practice.
>
> "QUERY" is currently translated. Your patch loses that.
I see; enclosed is v5 which fixes this.
The effective diff from the last one is:
-char *label = "QUERY";
+char *label = _("QUERY");
and
-label = psprintf("QUERY (\\%s)", curcmd);
+label = psprintf(_("QUERY (\\%s)"), curcmd);
Best,
David
v5-0001-Improve-SQL-comments-on-echo-hidden-output.patch
Description: Binary data
Re: Adding comments to help understand psql hidden queries
On 03.04.24 19:16, David Christensen wrote: I removed _() in the output of the query/stars since there'd be no sensible existing translations for the constructed string, which included the query string itself. If we need it for the "QUERY" string, this could be added fairly easily, but the existing piece would have been nonsensical and never used in practice. "QUERY" is currently translated. Your patch loses that.
Re: Adding comments to help understand psql hidden queries
I got Greg's blessing on squashing the commits down, and now including a v4 with additional improvements on the output formatting front. Main changes: - all generated comments are the same width - width has been bumped to 80 - removed _() functions for consumers of the new output functions This patch adds two new helper functions, OutputComment() and OutputCommentStars() to output and wrap the comments to the appropriate widths. Everything should continue to work just fine if we want to adjust the width by just adjusting the MAX_COMMENT_WIDTH symbol. I removed _() in the output of the query/stars since there'd be no sensible existing translations for the constructed string, which included the query string itself. If we need it for the "QUERY" string, this could be added fairly easily, but the existing piece would have been nonsensical and never used in practice. Thanks, David v4-0001-Improve-SQL-comments-on-echo-hidden-output.patch Description: Binary data
Re: Adding comments to help understand psql hidden queries
On Fri, Mar 22, 2024 at 11:39 AM David Christensen wrote: > I think it's easier to keep the widths balanced than constant (patch > version included here) Yeah, I'm fine with that, especially because nobody is translating it, nor are they likely to, to be honest. Cheers, Greg
Re: Adding comments to help understand psql hidden queries
On Fri, Mar 22, 2024 at 9:47 AM Greg Sabino Mullane wrote: > > On Thu, Mar 21, 2024 at 6:20 PM Peter Eisentraut wrote: >> >> lines are supposed to align vertically. With your patch, the first line >> would have variable length depending on the command. > > > Yes, that is a good point. Aligning those would be quite tricky, what if we > just kept a standard width for the closing query? Probably the 24 stars we > currently have to match "QUERY", which it appears nobody has changed for > translation purposes yet anyway. (If I am reading the code correctly, it > would be up to the translators to maintain the vertical alignment). I think it's easier to keep the widths balanced than constant (patch version included here), but if we needed to squeeze the opening string to a standard width that would be possible without too much trouble. The internal comment strings seem to be a bit more of a pain if we wanted all of the comments to be the same width, as we'd need a table or something so we can compute the longest string width, etc; doesn't seem worth the convolutions IMHO. No changes to Greg's patch, just keeping 'em both so cfbot stays happy. David v3-0002-Add-output-of-the-command-that-got-us-here-to-the.patch Description: Binary data v3-0001-Include-SQL-comments-on-echo-hidden-output.patch Description: Binary data
Re: Adding comments to help understand psql hidden queries
On Thu, Mar 21, 2024 at 6:20 PM Peter Eisentraut wrote: > lines are supposed to align vertically. With your patch, the first line > would have variable length depending on the command. > Yes, that is a good point. Aligning those would be quite tricky, what if we just kept a standard width for the closing query? Probably the 24 stars we currently have to match "QUERY", which it appears nobody has changed for translation purposes yet anyway. (If I am reading the code correctly, it would be up to the translators to maintain the vertical alignment). Cheers, Greg
Re: Adding comments to help understand psql hidden queries
On 21.03.24 18:31, David Christensen wrote: Thanks for the feedback. Enclosed is a v2 of this series(?) rebased and with that warning fixed; @Greg Sabino Mullane I just created a commit on your behalf with the message to hackers. I'm also creating a commit-fest entry for this thread. I don't think your patch takes into account that the /**... QUERY ...**/ ... /**... ...**/ lines are supposed to align vertically. With your patch, the first line would have variable length depending on the command.
Re: Adding comments to help understand psql hidden queries
Created the CF entry in commitfest 48 but didn't see it was already in 47; closing the CFEntry in 48. (Doesn't appear to be a different status than "Withdrawn"...)
Re: Adding comments to help understand psql hidden queries
Hi Jim, Thanks for the feedback. Enclosed is a v2 of this series(?) rebased and with that warning fixed; @Greg Sabino Mullane I just created a commit on your behalf with the message to hackers. I'm also creating a commit-fest entry for this thread. Best, David v2-0002-Add-output-of-the-command-that-got-us-here-to-the.patch Description: Binary data v2-0001-Include-SQL-comments-on-echo-hidden-output.patch Description: Binary data
Re: Adding comments to help understand psql hidden queries
Hi Greg, hi David
On 01.02.24 23:39, David Christensen wrote:
> On Thu, Feb 1, 2024 at 4:34 PM Greg Sabino Mullane wrote:
>> The use of the --echo-hidden flag in psql is used to show people the way
>> psql performs its magic for its backslash commands. None of them has more
>> magic than "\d relation", but it suffers from needing a lot of separate
>> queries to gather all of the information it needs. Unfortunately, those
>> queries can get overwhelming and hard to figure out which one does what,
>> especially for those not already very familiar with the system catalogs.
>> Attached is a patch to add a small SQL comment to the top of each SELECT
>> query inside describeOneTableDetail. All other functions use a single query,
>> and thus need no additional context. But "\d mytable" has the potential to
>> run over a dozen SQL queries! The new format looks like this:
>>
>> / QUERY */
>> /* Get information about row-level policies */
>> SELECT pol.polname, pol.polpermissive,
>> CASE WHEN pol.polroles = '{0}' THEN NULL ELSE
>> pg_catalog.array_to_string(array(select rolname from pg_catalog.pg_roles
>> where oid = any (pol.polroles) order by 1),',') END,
>> pg_catalog.pg_get_expr(pol.polqual, pol.polrelid),
>> pg_catalog.pg_get_expr(pol.polwithcheck, pol.polrelid),
>> CASE pol.polcmd
>> WHEN 'r' THEN 'SELECT'
>> WHEN 'a' THEN 'INSERT'
>> WHEN 'w' THEN 'UPDATE'
>> WHEN 'd' THEN 'DELETE'
>> END AS cmd
>> FROM pg_catalog.pg_policy pol
>> WHERE pol.polrelid = '134384' ORDER BY 1;
>> //
>>
>> Cheers,
>> Greg
> Thanks, this looks like some helpful information. In the same vein,
> I'm including a patch which adds information about the command that
> generates the given query as well (atop your commit). This will
> modify the query line to include the command itself:
>
> / QUERY (\dRs) */
>
> Best,
>
> David
Having this kind of information in each query would have saved me a lot
of time in the past :) +1
There is a tiny little issue in the last patch (qualifiers):
command.c:312:16: warning: assignment discards ‘const’ qualifier from
pointer target type [-Wdiscarded-qualifiers]
312 | curcmd = cmd;
Thanks
--
Jim
Re: Adding comments to help understand psql hidden queries
On Thu, Feb 1, 2024 at 4:34 PM Greg Sabino Mullane wrote:
>
> The use of the --echo-hidden flag in psql is used to show people the way psql
> performs its magic for its backslash commands. None of them has more magic
> than "\d relation", but it suffers from needing a lot of separate queries to
> gather all of the information it needs. Unfortunately, those queries can get
> overwhelming and hard to figure out which one does what, especially for those
> not already very familiar with the system catalogs. Attached is a patch to
> add a small SQL comment to the top of each SELECT query inside
> describeOneTableDetail. All other functions use a single query, and thus need
> no additional context. But "\d mytable" has the potential to run over a dozen
> SQL queries! The new format looks like this:
>
> / QUERY */
> /* Get information about row-level policies */
> SELECT pol.polname, pol.polpermissive,
> CASE WHEN pol.polroles = '{0}' THEN NULL ELSE
> pg_catalog.array_to_string(array(select rolname from pg_catalog.pg_roles
> where oid = any (pol.polroles) order by 1),',') END,
> pg_catalog.pg_get_expr(pol.polqual, pol.polrelid),
> pg_catalog.pg_get_expr(pol.polwithcheck, pol.polrelid),
> CASE pol.polcmd
> WHEN 'r' THEN 'SELECT'
> WHEN 'a' THEN 'INSERT'
> WHEN 'w' THEN 'UPDATE'
> WHEN 'd' THEN 'DELETE'
> END AS cmd
> FROM pg_catalog.pg_policy pol
> WHERE pol.polrelid = '134384' ORDER BY 1;
> //
>
> Cheers,
> Greg
Thanks, this looks like some helpful information. In the same vein,
I'm including a patch which adds information about the command that
generates the given query as well (atop your commit). This will
modify the query line to include the command itself:
/ QUERY (\dRs) */
Best,
David
0001-Add-output-of-the-command-that-got-us-here-to-the-QU.patch
Description: Binary data
