public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Christensen <[email protected]>
To: Greg Sabino Mullane <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Jim Jones <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Adding comments to help understand psql hidden queries
Date: Fri, 22 Mar 2024 10:39:10 -0500
Message-ID: <CAHM0NXjO-Thdj+DqT5SOfLGVP-aVKhguiRfmkLDu4BJ_H-gg0A@mail.gmail.com> (raw)
In-Reply-To: <CAKAnmmL6-U+QPsuWM0r8mNq0Lo80d8L=cR7JLk7pypXJfwt=EA@mail.gmail.com>
References: <CAKAnmmJz8Hh=8Ru8jgzySPWmLBhnv4=oc_0KRiz-UORJ0Dex+w@mail.gmail.com>
<CAHM0NXiD15-23=r1B9rHzKhS8e0+zwp_a6PncdLNetyF0xpNpA@mail.gmail.com>
<[email protected]>
<CAHM0NXheC3YO=ANPX0fCZKcQxMVBdgDzk9fc=-4TTCUN0aLYWA@mail.gmail.com>
<[email protected]>
<CAKAnmmL6-U+QPsuWM0r8mNq0Lo80d8L=cR7JLk7pypXJfwt=EA@mail.gmail.com>
On Fri, Mar 22, 2024 at 9:47 AM Greg Sabino Mullane <[email protected]> wrote:
>
> On Thu, Mar 21, 2024 at 6:20 PM Peter Eisentraut <[email protected]> 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
Attachments:
[application/octet-stream] v3-0002-Add-output-of-the-command-that-got-us-here-to-the.patch (2.9K, 2-v3-0002-Add-output-of-the-command-that-got-us-here-to-the.patch)
download | inline diff:
From 83f2586c6a28f237a8e81a9d846b44975998307e Mon Sep 17 00:00:00 2001
From: David Christensen <[email protected]>
Date: Thu, 21 Mar 2024 12:27:32 -0500
Subject: [PATCH v3 2/2] Add output of the command that got us here to the
QUERY output
---
src/bin/psql/command.c | 5 +++++
src/bin/psql/common.c | 24 ++++++++++++++++++++----
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 9b0fa041f7..4cfb91e134 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -56,6 +56,8 @@ typedef enum EditableObjectType
EditableView,
} EditableObjectType;
+char *curcmd = NULL;
+
/* local function declarations */
static backslashResult exec_command(const char *cmd,
PsqlScanState scan_state,
@@ -307,6 +309,7 @@ exec_command(const char *cmd,
cmd);
}
+ curcmd = (char *)cmd;
if (strcmp(cmd, "a") == 0)
status = exec_command_a(scan_state, active_branch);
else if (strcmp(cmd, "bind") == 0)
@@ -423,6 +426,8 @@ exec_command(const char *cmd,
else
status = PSQL_CMD_UNKNOWN;
+ curcmd = NULL;
+
/*
* All the commands that return PSQL_CMD_SEND want to execute previous_buf
* if query_buf is empty. For convenience we implement that here, not in
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 76e01b02a3..18c8b067ee 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -42,6 +42,7 @@ static int ExecQueryAndProcessResults(const char *query,
static bool command_no_begin(const char *query);
static bool is_select_command(const char *query);
+extern char *curcmd;
/*
* openQueryOutputFile --- attempt to open a query output file
@@ -581,6 +582,12 @@ PGresult *
PSQLexec(const char *query)
{
PGresult *res;
+ char *label = "";
+ const char *asterisks = "********************"; /* ensure this is as long as the
+ * longest command that might be
+ * displayed */
+ int curcmd_length = 0;
+
if (!pset.db)
{
@@ -588,21 +595,30 @@ PSQLexec(const char *query)
return NULL;
}
+ if (curcmd)
+ {
+ label = psprintf(" (\\%s)", curcmd);
+ curcmd_length = strlen(label);
+ }
+
if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF)
{
- printf(_("/******** QUERY *********/\n"
+ printf(_("/******** QUERY%s *********/\n"
"%s\n"
- "/************************/\n\n"), query);
+ "/************************%.*s/\n\n"), label, query, curcmd_length, asterisks);
fflush(stdout);
if (pset.logfile)
{
fprintf(pset.logfile,
- _("/******** QUERY *********/\n"
+ _("/******** QUERY%s *********/\n"
"%s\n"
- "/************************/\n\n"), query);
+ "/************************%.*s/\n\n"), label, query, curcmd_length, asterisks);
fflush(pset.logfile);
}
+ if (curcmd)
+ pfree(label);
+
if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC)
return NULL;
}
--
2.39.3 (Apple Git-146)
[application/octet-stream] v3-0001-Include-SQL-comments-on-echo-hidden-output.patch (14.6K, 3-v3-0001-Include-SQL-comments-on-echo-hidden-output.patch)
download | inline diff:
From 3f511da8c5fdc4a89ecc09a4d5b9b8eb76fee80f Mon Sep 17 00:00:00 2001
From: Greg Sabino Mullane <[email protected]>
Date: Thu, 21 Mar 2024 12:12:54 -0500
Subject: [PATCH v3 1/2] Include SQL comments on --echo-hidden output
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;
/************************/
---
src/bin/psql/describe.c | 82 ++++++++++++++++++++++++++---------------
1 file changed, 52 insertions(+), 30 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 68b2ea8872..7f272b2ac1 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1586,9 +1586,10 @@ describeOneTableDetails(const char *schemaname,
initPQExpBuffer(&tmpbuf);
/* Get general table info */
+ printfPQExpBuffer(&buf, _("/* Get general table information */\n"));
if (pset.sversion >= 120000)
{
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT c.relchecks, c.relkind, c.relhasindex, c.relhasrules, "
"c.relhastriggers, c.relrowsecurity, c.relforcerowsecurity, "
"false AS relhasoids, c.relispartition, %s, c.reltablespace, "
@@ -1606,7 +1607,7 @@ describeOneTableDetails(const char *schemaname,
}
else if (pset.sversion >= 100000)
{
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT c.relchecks, c.relkind, c.relhasindex, c.relhasrules, "
"c.relhastriggers, c.relrowsecurity, c.relforcerowsecurity, "
"c.relhasoids, c.relispartition, %s, c.reltablespace, "
@@ -1623,7 +1624,7 @@ describeOneTableDetails(const char *schemaname,
}
else if (pset.sversion >= 90500)
{
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT c.relchecks, c.relkind, c.relhasindex, c.relhasrules, "
"c.relhastriggers, c.relrowsecurity, c.relforcerowsecurity, "
"c.relhasoids, false as relispartition, %s, c.reltablespace, "
@@ -1640,7 +1641,7 @@ describeOneTableDetails(const char *schemaname,
}
else if (pset.sversion >= 90400)
{
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT c.relchecks, c.relkind, c.relhasindex, c.relhasrules, "
"c.relhastriggers, false, false, c.relhasoids, "
"false as relispartition, %s, c.reltablespace, "
@@ -1657,7 +1658,7 @@ describeOneTableDetails(const char *schemaname,
}
else
{
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT c.relchecks, c.relkind, c.relhasindex, c.relhasrules, "
"c.relhastriggers, false, false, c.relhasoids, "
"false as relispartition, %s, c.reltablespace, "
@@ -1718,9 +1719,10 @@ describeOneTableDetails(const char *schemaname,
printQueryOpt myopt = pset.popt;
char *footers[2] = {NULL, NULL};
+ printfPQExpBuffer(&buf, _("/* Get general sequence information */\n"));
if (pset.sversion >= 100000)
{
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT pg_catalog.format_type(seqtypid, NULL) AS \"%s\",\n"
" seqstart AS \"%s\",\n"
" seqmin AS \"%s\",\n"
@@ -1744,7 +1746,7 @@ describeOneTableDetails(const char *schemaname,
}
else
{
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT 'bigint' AS \"%s\",\n"
" start_value AS \"%s\",\n"
" min_value AS \"%s\",\n"
@@ -1771,7 +1773,8 @@ describeOneTableDetails(const char *schemaname,
goto error_return;
/* Get the column that owns this sequence */
- printfPQExpBuffer(&buf, "SELECT pg_catalog.quote_ident(nspname) || '.' ||"
+ printfPQExpBuffer(&buf, _("/* Get the column that owns this sequence */\n"));
+ appendPQExpBuffer(&buf, "SELECT pg_catalog.quote_ident(nspname) || '.' ||"
"\n pg_catalog.quote_ident(relname) || '.' ||"
"\n pg_catalog.quote_ident(attname),"
"\n d.deptype"
@@ -1850,7 +1853,8 @@ describeOneTableDetails(const char *schemaname,
* duplicative test logic below.
*/
cols = 0;
- printfPQExpBuffer(&buf, "SELECT a.attname");
+ printfPQExpBuffer(&buf, _("/* Get information about each column */\n"));
+ appendPQExpBuffer(&buf, "SELECT a.attname");
attname_col = cols++;
appendPQExpBufferStr(&buf, ",\n pg_catalog.format_type(a.atttypid, a.atttypmod)");
atttype_col = cols++;
@@ -2150,7 +2154,8 @@ describeOneTableDetails(const char *schemaname,
/* Footer information for a partition child table */
PGresult *result;
- printfPQExpBuffer(&buf,
+ printfPQExpBuffer(&buf, _("/* Get partition information for this table */\n"));
+ appendPQExpBuffer(&buf,
"SELECT inhparent::pg_catalog.regclass,\n"
" pg_catalog.pg_get_expr(c.relpartbound, c.oid),\n ");
@@ -2205,7 +2210,8 @@ describeOneTableDetails(const char *schemaname,
/* Footer information for a partitioned table (partitioning parent) */
PGresult *result;
- printfPQExpBuffer(&buf,
+ printfPQExpBuffer(&buf, _("/* Get the partition key for this table */\n"));
+ appendPQExpBuffer(&buf,
"SELECT pg_catalog.pg_get_partkeydef('%s'::pg_catalog.oid);",
oid);
result = PSQLexec(buf.data);
@@ -2227,7 +2233,8 @@ describeOneTableDetails(const char *schemaname,
/* For a TOAST table, print name of owning table */
PGresult *result;
- printfPQExpBuffer(&buf,
+ printfPQExpBuffer(&buf, _("/* Find which table owns this TOAST table */\n"));
+ appendPQExpBuffer(&buf,
"SELECT n.nspname, c.relname\n"
"FROM pg_catalog.pg_class c"
" JOIN pg_catalog.pg_namespace n"
@@ -2255,7 +2262,8 @@ describeOneTableDetails(const char *schemaname,
/* Footer information about an index */
PGresult *result;
- printfPQExpBuffer(&buf,
+ printfPQExpBuffer(&buf, _("/* Get information about this index */\n"));
+ appendPQExpBuffer(&buf,
"SELECT i.indisunique, i.indisprimary, i.indisclustered, "
"i.indisvalid,\n"
" (NOT i.indimmediate) AND "
@@ -2372,7 +2380,8 @@ describeOneTableDetails(const char *schemaname,
/* print indexes */
if (tableinfo.hasindex)
{
- printfPQExpBuffer(&buf,
+ printfPQExpBuffer(&buf, _("/* Get information about each index */\n"));
+ appendPQExpBuffer(&buf,
"SELECT c2.relname, i.indisprimary, i.indisunique, "
"i.indisclustered, i.indisvalid, "
"pg_catalog.pg_get_indexdef(i.indexrelid, 0, true),\n "
@@ -2510,6 +2519,8 @@ describeOneTableDetails(const char *schemaname,
if (tableinfo.hastriggers ||
tableinfo.relkind == RELKIND_PARTITIONED_TABLE)
{
+ printfPQExpBuffer(&buf, _("/* Get information about foreign key constraints */\n"));
+
if (pset.sversion >= 120000 &&
(tableinfo.ispartition || tableinfo.relkind == RELKIND_PARTITIONED_TABLE))
{
@@ -2517,7 +2528,7 @@ describeOneTableDetails(const char *schemaname,
* Put the constraints defined in this table first, followed
* by the constraints defined in ancestor partitioned tables.
*/
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT conrelid = '%s'::pg_catalog.regclass AS sametable,\n"
" conname,\n"
" pg_catalog.pg_get_constraintdef(oid, true) AS condef,\n"
@@ -2530,7 +2541,7 @@ describeOneTableDetails(const char *schemaname,
}
else
{
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT true as sametable, conname,\n"
" pg_catalog.pg_get_constraintdef(r.oid, true) as condef,\n"
" conrelid::pg_catalog.regclass AS ontable\n"
@@ -2584,9 +2595,10 @@ describeOneTableDetails(const char *schemaname,
if (tableinfo.hastriggers ||
tableinfo.relkind == RELKIND_PARTITIONED_TABLE)
{
+ printfPQExpBuffer(&buf, _("/* Get information about incoming foreign key references */\n"));
if (pset.sversion >= 120000)
{
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT conname, conrelid::pg_catalog.regclass AS ontable,\n"
" pg_catalog.pg_get_constraintdef(oid, true) AS condef\n"
" FROM pg_catalog.pg_constraint c\n"
@@ -2598,7 +2610,7 @@ describeOneTableDetails(const char *schemaname,
}
else
{
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT conname, conrelid::pg_catalog.regclass AS ontable,\n"
" pg_catalog.pg_get_constraintdef(oid, true) AS condef\n"
" FROM pg_catalog.pg_constraint\n"
@@ -2636,7 +2648,8 @@ describeOneTableDetails(const char *schemaname,
/* print any row-level policies */
if (pset.sversion >= 90500)
{
- printfPQExpBuffer(&buf, "SELECT pol.polname,");
+ printfPQExpBuffer(&buf, _("/* Get information about row-level policies */\n"));
+ appendPQExpBuffer(&buf, "SELECT pol.polname,");
if (pset.sversion >= 100000)
appendPQExpBufferStr(&buf,
" pol.polpermissive,\n");
@@ -2716,9 +2729,10 @@ describeOneTableDetails(const char *schemaname,
}
/* print any extended statistics */
+ printfPQExpBuffer(&buf, _("/* Get information about extended statistics */\n"));
if (pset.sversion >= 140000)
{
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT oid, "
"stxrelid::pg_catalog.regclass, "
"stxnamespace::pg_catalog.regnamespace::pg_catalog.text AS nsp, "
@@ -2815,7 +2829,7 @@ describeOneTableDetails(const char *schemaname,
}
else if (pset.sversion >= 100000)
{
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT oid, "
"stxrelid::pg_catalog.regclass, "
"stxnamespace::pg_catalog.regnamespace AS nsp, "
@@ -2894,7 +2908,8 @@ describeOneTableDetails(const char *schemaname,
/* print rules */
if (tableinfo.hasrules && tableinfo.relkind != RELKIND_MATVIEW)
{
- printfPQExpBuffer(&buf,
+ printfPQExpBuffer(&buf, _("/* Get information about each rule for this table */\n"));
+ appendPQExpBuffer(&buf,
"SELECT r.rulename, trim(trailing ';' from pg_catalog.pg_get_ruledef(r.oid, true)), "
"ev_enabled\n"
"FROM pg_catalog.pg_rewrite r\n"
@@ -2977,9 +2992,10 @@ describeOneTableDetails(const char *schemaname,
/* print any publications */
if (pset.sversion >= 100000)
{
+ printfPQExpBuffer(&buf, _("/* Get information about each publication using this table */\n"));
if (pset.sversion >= 150000)
{
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT pubname\n"
" , NULL\n"
" , NULL\n"
@@ -3011,7 +3027,7 @@ describeOneTableDetails(const char *schemaname,
}
else
{
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT pubname\n"
" , NULL\n"
" , NULL\n"
@@ -3061,7 +3077,9 @@ describeOneTableDetails(const char *schemaname,
/* If verbose, print NOT NULL constraints */
if (verbose)
{
- printfPQExpBuffer(&buf,
+ printfPQExpBuffer(&buf, _("/* Get information about NOT NULL constraints */\n"));
+ appendPQExpBuffer(&buf,
+ "/* Find NOT NULL constraints */\n"
"SELECT co.conname, at.attname, co.connoinherit, co.conislocal,\n"
"co.coninhcount <> 0\n"
"FROM pg_catalog.pg_constraint co JOIN\n"
@@ -3133,7 +3151,8 @@ describeOneTableDetails(const char *schemaname,
/* print rules */
if (tableinfo.hasrules)
{
- printfPQExpBuffer(&buf,
+ printfPQExpBuffer(&buf, _("/* Get information about each rule for this view */\n"));
+ appendPQExpBuffer(&buf,
"SELECT r.rulename, trim(trailing ';' from pg_catalog.pg_get_ruledef(r.oid, true))\n"
"FROM pg_catalog.pg_rewrite r\n"
"WHERE r.ev_class = '%s' AND r.rulename != '_RETURN' ORDER BY 1;",
@@ -3170,7 +3189,8 @@ describeOneTableDetails(const char *schemaname,
PGresult *result;
int tuples;
- printfPQExpBuffer(&buf,
+ printfPQExpBuffer(&buf, _("/* Get information about each trigger on this table */\n"));
+ appendPQExpBuffer(&buf,
"SELECT t.tgname, "
"pg_catalog.pg_get_triggerdef(t.oid, true), "
"t.tgenabled, t.tgisinternal,\n");
@@ -3389,6 +3409,7 @@ describeOneTableDetails(const char *schemaname,
/* print tables inherited from (exclude partitioned parents) */
printfPQExpBuffer(&buf,
+ "/* Find tables inherited from */\n"
"SELECT c.oid::pg_catalog.regclass\n"
"FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i\n"
"WHERE c.oid = i.inhparent AND i.inhrelid = '%s'\n"
@@ -3425,8 +3446,9 @@ describeOneTableDetails(const char *schemaname,
}
/* print child tables (with additional info if partitions) */
+ printfPQExpBuffer(&buf, _("/* Get information about child tables */\n"));
if (pset.sversion >= 140000)
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT c.oid::pg_catalog.regclass, c.relkind,"
" inhdetachpending,"
" pg_catalog.pg_get_expr(c.relpartbound, c.oid)\n"
@@ -3436,7 +3458,7 @@ describeOneTableDetails(const char *schemaname,
" c.oid::pg_catalog.regclass::pg_catalog.text;",
oid);
else if (pset.sversion >= 100000)
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT c.oid::pg_catalog.regclass, c.relkind,"
" false AS inhdetachpending,"
" pg_catalog.pg_get_expr(c.relpartbound, c.oid)\n"
@@ -3446,7 +3468,7 @@ describeOneTableDetails(const char *schemaname,
" c.oid::pg_catalog.regclass::pg_catalog.text;",
oid);
else
- printfPQExpBuffer(&buf,
+ appendPQExpBuffer(&buf,
"SELECT c.oid::pg_catalog.regclass, c.relkind,"
" false AS inhdetachpending, NULL\n"
"FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i\n"
--
2.39.3 (Apple Git-146)
view thread (12+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Adding comments to help understand psql hidden queries
In-Reply-To: <CAHM0NXjO-Thdj+DqT5SOfLGVP-aVKhguiRfmkLDu4BJ_H-gg0A@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox