From e92896202661f9f4d7707bbc33b934d4f2e2529a Mon Sep 17 00:00:00 2001 From: David Christensen Date: Wed, 3 Apr 2024 13:06:56 -0400 Subject: [PATCH v4] Improve 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! For _all_ query output types (not just \d), we include the backslash command that was used to generate the given query. The new format looks like this: /********************************* QUERY (\d) **********************************/ /* Get information about child tables */ SELECT c.oid::pg_catalog.regclass, c.relkind, inhdetachpending, pg_catalog.pg_get_expr(c.relpartbound, c.oid) FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i WHERE c.oid = i.inhrelid AND i.inhparent = '76992' ORDER BY pg_catalog.pg_get_expr(c.relpartbound, c.oid) = 'DEFAULT', c.oid::pg_catalog.regclass::pg_catalog.text; /*******************************************************************************/ This includes some level of refactoring the output routines in PSQLexec(), adding OutputComment() and OutputCommentStars() to share a single output width. (This could presumably be hoisted into something besides `psql`, but since this is the only consumer here, leaving this for now.) Since we are lining up all of the comments that are being output, we expand the comment width to 80 chars. Co-authored-by: Greg Sabino Mullane Co-authored-by: David Christensen --- src/bin/psql/command.c | 5 ++ src/bin/psql/common.c | 125 +++++++++++++++++++++++++++++++++++++--- src/bin/psql/common.h | 5 ++ src/bin/psql/describe.c | 81 ++++++++++++++++---------- 4 files changed, 179 insertions(+), 37 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..3e94bd5d12 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,8 @@ PGresult * PSQLexec(const char *query) { PGresult *res; + char *label = "QUERY"; + if (!pset.db) { @@ -588,21 +591,32 @@ PSQLexec(const char *query) return NULL; } + if (curcmd) + label = psprintf("QUERY (\\%s)", curcmd); + if (pset.echo_hidden != PSQL_ECHO_HIDDEN_OFF) { - printf(_("/******** QUERY *********/\n" - "%s\n" - "/************************/\n\n"), query); + PQExpBufferData buf; + initPQExpBuffer(&buf); + + OutputCommentStars(&buf, label); + appendPQExpBufferStr(&buf, query); + appendPQExpBufferChar(&buf, '\n'); + OutputCommentStars(&buf, NULL); + + printf("%s", buf.data); fflush(stdout); if (pset.logfile) { - fprintf(pset.logfile, - _("/******** QUERY *********/\n" - "%s\n" - "/************************/\n\n"), query); + fprintf(pset.logfile, "%s", buf.data); fflush(pset.logfile); } + pfree(buf.data); + + if (curcmd) + pfree(label); + if (pset.echo_hidden == PSQL_ECHO_HIDDEN_NOEXEC) return NULL; } @@ -2434,3 +2448,100 @@ recognized_connection_string(const char *connstr) { return uri_prefix_length(connstr) != 0 || strchr(connstr, '=') != NULL; } + +/* + * OutputComment() outputs the given string to a buffer as a + * fixed width comment, wrapping the text given to the proper size and + * breaking at a space. We assume there are no inner comments that need to be + * escaped. + */ +void +OutputComment(PQExpBufferData *buf, const char *string) +{ + int len = strlen(string); + int lineStart = 0; + int lineEnd = MAX_COMMENT_WIDTH - 6; // Accounting for " * " at the start and " *" at the end of each line + static char spaces[MAX_COMMENT_WIDTH] = {0}; + + if (spaces[0] != ' ') + memset(spaces, ' ', MAX_COMMENT_WIDTH); + + while (lineStart < len) { + // Adjust lineEnd if it's beyond the length of the string + if (lineEnd >= len) { + lineEnd = len - 1; + } else { + // Ensure we break the line at a space (if possible) to avoid breaking words + while (lineEnd > lineStart && string[lineEnd] != ' ') { + lineEnd--; + } + if (lineEnd == lineStart) { + // If we couldn't find a space, force a break at the maximum line width + lineEnd = lineStart + MAX_COMMENT_WIDTH - 6; + } + } + + // output our data for the width we have, including the piece of the comment + printfPQExpBuffer(buf, + "/* %.*s%.*s */\n", + lineEnd - lineStart + 1, /* number of chars to output */ + string + lineStart, /* offset of chars to copy */ + /* length of padding */ + MAX_COMMENT_WIDTH - 6 - (lineEnd - lineStart), + spaces + ); + + // Move to the next line + lineStart = lineEnd + 1; + lineEnd = lineStart + MAX_COMMENT_WIDTH - 6; + } +} + + +/* + * OutputCommentStars() outputs the given single-line string wrapped in stars + * to the given width. + */ +void +OutputCommentStars(PQExpBufferData *buf, const char *string) +{ + int len = string ? strlen(string) : 0; + char stars[MAX_COMMENT_WIDTH + 3]; + int startOutputOffset; + + /* This shouldn't happen based on current callers, but we'll truncate to a + * safe size if need be. */ + + if (len > MAX_COMMENT_WIDTH) /* space for opening comment, closing comment, and spaces */ + len = MAX_COMMENT_WIDTH; + + /* If we have a zero-width string then this is a special-case where we + * just want a line of stars. To minimize code disruption in this case, + * we'll just set startOutputOffset to a value larger than + * MAX_COMMENT_WIDTH. */ + + if (len > 0) + startOutputOffset = (((MAX_COMMENT_WIDTH - len - 2) / 2)); /* extra for space */ + else + startOutputOffset = MAX_COMMENT_WIDTH * 2; + + stars[0] = '/'; + stars[MAX_COMMENT_WIDTH] = '/'; + stars[MAX_COMMENT_WIDTH+1] = '\n'; + stars[MAX_COMMENT_WIDTH+2] = '\0'; + + for (int i = 1; i < MAX_COMMENT_WIDTH; i++) + { + if (i >= startOutputOffset && i <= startOutputOffset + len + 1) + { + if (i == startOutputOffset || i == startOutputOffset + len + 1) + stars[i] = ' '; + else + stars[i] = string[i - startOutputOffset - 1]; + } + else + stars[i] = '*'; + } + + appendPQExpBufferStr(buf, stars); +} diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h index 6efe12274f..c16cbc6e6f 100644 --- a/src/bin/psql/common.h +++ b/src/bin/psql/common.h @@ -15,6 +15,8 @@ #include "fe_utils/psqlscan.h" #include "libpq-fe.h" +#define MAX_COMMENT_WIDTH 80 + extern bool openQueryOutputFile(const char *fname, FILE **fout, bool *is_pipe); extern bool setQFout(const char *fname); @@ -44,4 +46,7 @@ extern void expand_tilde(char **filename); extern bool recognized_connection_string(const char *connstr); +extern void OutputComment(PQExpBufferData *buf, const char *string); +extern void OutputCommentStars(PQExpBufferData *buf, const char *string); + #endif /* COMMON_H */ diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c index 68b2ea8872..53b8fdd469 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 */ + OutputComment(&buf, _("Get general table information")); 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}; + OutputComment(&buf, _("Get general sequence information *")); 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) || '.' ||" + OutputComment(&buf, _("Get the column that owns this sequence")); + 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"); + OutputComment(&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, + OutputComment(&buf, _("Get partition information for this table")); + 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, + OutputComment(&buf, _("Get the partition key for this table")); + 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, + OutputComment(&buf, _("Find which table owns this TOAST table")); + 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, + OutputComment(&buf, _("Get information about this index")); + 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, + OutputComment(&buf, _("Get information about each index")); + 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) { + OutputComment(&buf, _("Get information about foreign key constraints")); + 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) { + OutputComment(&buf, _("Get information about incoming foreign key references")); 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,"); + OutputComment(&buf, _("Get information about row-level policies")); + 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 */ + OutputComment(&buf, _("Get information about extended statistics")); 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, + OutputComment(&buf, _("Get information about each rule for this table")); + 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) { + OutputComment(&buf, _("Get information about each publication using this table")); 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,8 @@ describeOneTableDetails(const char *schemaname, /* If verbose, print NOT NULL constraints */ if (verbose) { - printfPQExpBuffer(&buf, + OutputComment(&buf, _("Get information about NOT NULL constraints")); + appendPQExpBuffer(&buf, "SELECT co.conname, at.attname, co.connoinherit, co.conislocal,\n" "co.coninhcount <> 0\n" "FROM pg_catalog.pg_constraint co JOIN\n" @@ -3133,7 +3150,8 @@ describeOneTableDetails(const char *schemaname, /* print rules */ if (tableinfo.hasrules) { - printfPQExpBuffer(&buf, + OutputComment(&buf, _("Get information about each rule for this view")); + 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 +3188,8 @@ describeOneTableDetails(const char *schemaname, PGresult *result; int tuples; - printfPQExpBuffer(&buf, + OutputComment(&buf, _("Get information about each trigger on this table")); + appendPQExpBuffer(&buf, "SELECT t.tgname, " "pg_catalog.pg_get_triggerdef(t.oid, true), " "t.tgenabled, t.tgisinternal,\n"); @@ -3388,6 +3407,7 @@ describeOneTableDetails(const char *schemaname, } /* print tables inherited from (exclude partitioned parents) */ + OutputComment(&buf, _("Find tables inherited from")); printfPQExpBuffer(&buf, "SELECT c.oid::pg_catalog.regclass\n" "FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i\n" @@ -3425,8 +3445,9 @@ describeOneTableDetails(const char *schemaname, } /* print child tables (with additional info if partitions) */ + OutputComment(&buf, _("Get information about child tables")); 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 +3457,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 +3467,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.40.1