Thread: Adding comments to help understand psql hidden queries

On Thu, Feb 1, 2024 at 4:34 PM Greg Sabino Mullane <htamfids@gmail.com> 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
gatherall of the information it needs. Unfortunately, those queries can get overwhelming and hard to figure out which
onedoes what, especially for those not already very familiar with the system catalogs. Attached is a patch to add a
smallSQL comment to the top of each SELECT query inside describeOneTableDetail. All other functions use a single query,
andthus need no additional context. But "\d mytable" has the potential to run over a dozen SQL queries! The new format
lookslike 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_roleswhere 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

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 <htamfids@gmail.com> 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
gatherall of the information it needs. Unfortunately, those queries can get overwhelming and hard to figure out which
onedoes what, especially for those not already very familiar with the system catalogs. Attached is a patch to add a
smallSQL comment to the top of each SELECT query inside describeOneTableDetail. All other functions use a single query,
andthus need no additional context. But "\d mytable" has the potential to run over a dozen SQL queries! The new format
lookslike 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_roleswhere 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

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

Created the CF entry in commitfest 48 but didn't see it was already in 47; closing the CFEntry in 48. (Doesn't appear
tobe a different status than "Withdrawn"...) 

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.

On Fri, Mar 22, 2024 at 9:47 AM Greg Sabino Mullane <htamfids@gmail.com> wrote:
>
> On Thu, Mar 21, 2024 at 6:20 PM Peter Eisentraut <peter@eisentraut.org> 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
closingquery? Probably the 24 stars we currently have to match "QUERY", which it appears nobody has changed for
translationpurposes yet anyway. (If I am reading the code correctly, it would be up to the translators to maintain the
verticalalignment). 

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

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

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.

On Thu, Apr 4, 2024 at 9:32 AM Peter Eisentraut <peter@eisentraut.org> 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

On Thu, Apr 4, 2024 at 11:12 AM David Christensen <david+pg@pgguru.net> wrote:
>
> On Thu, Apr 4, 2024 at 9:32 AM Peter Eisentraut <peter@eisentraut.org> 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

On Sat, Jan 18, 2025 at 2:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Christensen <david+pg@pgguru.net> writes:
> > Any further concerns/issues with this patch that I can address to help
> > move it forward?
>
> I got around to looking at this finally --- sorry that it's been on
> the back burner for so long.  I think this is basically a good idea
> but it still requires a lot of sanding-down of rough edges.

Hi Tom, thanks for the detailed feedback.  I'll take your v6 and see
about adding the additional changes; I agree with what you've pointed
out at the high level, and will respond with additional questions of
my own if things seem ambiguous in terms of approach.

Thanks,

David