Hi all, This was on my stack of things for some time, but please find attached a patch to clean up some code with $subject because HEAD's describe.c is a mixed bag of relying on CppAsString2() and hardcoded values. Switching to CppAsString2() has the advantage to make the code more self-documented, so as it is not necessary to remember what a single byte means in a catalog.
I should have caught most of them, with exceptions related to policies, subscriptions and dependencies being intentional. I have noticed that describe.c includes pg_am.h and not pg_am_d.h. That's not a good idea even if it is OK now because this has the risk of pulling backend-side definitions into psql. psql -E reported consistent formats in the queries generated, so things look in rather good shape here. Note that there were a couple of value checks not part of the queries that relied on values from the catalogs for some relpersistences and replidents. I've fixed them while on it. Thoughts or comments are welcome. -- Michael
From 9fa620a80e3af543c134f1bfe5c547828680d204 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Tue, 22 Oct 2024 09:48:24 +0900 Subject: [PATCH] psql: Sprinkle more CppAsString2() in describe.c --- src/bin/psql/describe.c | 153 +++++++++++++++++++++++++--------------- 1 file changed, 97 insertions(+), 56 deletions(-) diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c index 363a66e718..a465a34afd 100644 --- a/src/bin/psql/describe.c +++ b/src/bin/psql/describe.c @@ -15,11 +15,17 @@ #include <ctype.h> -#include "catalog/pg_am.h" +#include "catalog/pg_am_d.h" +#include "catalog/pg_amop_d.h" #include "catalog/pg_attribute_d.h" #include "catalog/pg_cast_d.h" #include "catalog/pg_class_d.h" +#include "catalog/pg_collation_d.h" +#include "catalog/pg_constraint_d.h" #include "catalog/pg_default_acl_d.h" +#include "catalog/pg_statistic_ext_d.h" +#include "catalog/pg_proc_d.h" +#include "catalog/pg_type_d.h" #include "common.h" #include "common/logging.h" #include "describe.h" @@ -94,7 +100,7 @@ describeAggregates(const char *pattern, bool verbose, bool showSystem) " pg_catalog.obj_description(p.oid, 'pg_proc') as \"%s\"\n" "FROM pg_catalog.pg_proc p\n" " LEFT JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace\n" - "WHERE p.prokind = 'a'\n", + "WHERE p.prokind = " CppAsString2(PROKIND_AGGREGATE) "\n", gettext_noop("Description")); else appendPQExpBuffer(&buf, @@ -160,8 +166,8 @@ describeAccessMethods(const char *pattern, bool verbose) printfPQExpBuffer(&buf, "SELECT amname AS \"%s\",\n" " CASE amtype" - " WHEN 'i' THEN '%s'" - " WHEN 't' THEN '%s'" + " WHEN " CppAsString2(AMTYPE_INDEX) " THEN '%s'" + " WHEN " CppAsString2(AMTYPE_TABLE) " THEN '%s'" " END AS \"%s\"", gettext_noop("Name"), gettext_noop("Index"), @@ -340,9 +346,9 @@ describeFunctions(const char *functypes, const char *func_pattern, " pg_catalog.pg_get_function_result(p.oid) as \"%s\",\n" " pg_catalog.pg_get_function_arguments(p.oid) as \"%s\",\n" " CASE p.prokind\n" - " WHEN 'a' THEN '%s'\n" - " WHEN 'w' THEN '%s'\n" - " WHEN 'p' THEN '%s'\n" + " WHEN " CppAsString2(PROKIND_AGGREGATE) " THEN '%s'\n" + " WHEN " CppAsString2(PROKIND_WINDOW) " THEN '%s'\n" + " WHEN " CppAsString2(PROKIND_PROCEDURE) " THEN '%s'\n" " ELSE '%s'\n" " END as \"%s\"", gettext_noop("Result data type"), @@ -376,9 +382,12 @@ describeFunctions(const char *functypes, const char *func_pattern, { appendPQExpBuffer(&buf, ",\n CASE\n" - " WHEN p.provolatile = 'i' THEN '%s'\n" - " WHEN p.provolatile = 's' THEN '%s'\n" - " WHEN p.provolatile = 'v' THEN '%s'\n" + " WHEN p.provolatile = " + CppAsString2(PROVOLATILE_IMMUTABLE) " THEN '%s'\n" + " WHEN p.provolatile = " + CppAsString2(PROVOLATILE_STABLE) " THEN '%s'\n" + " WHEN p.provolatile = " + CppAsString2(PROVOLATILE_VOLATILE) " THEN '%s'\n" " END as \"%s\"", gettext_noop("immutable"), gettext_noop("stable"), @@ -387,9 +396,12 @@ describeFunctions(const char *functypes, const char *func_pattern, if (pset.sversion >= 90600) appendPQExpBuffer(&buf, ",\n CASE\n" - " WHEN p.proparallel = 'r' THEN '%s'\n" - " WHEN p.proparallel = 's' THEN '%s'\n" - " WHEN p.proparallel = 'u' THEN '%s'\n" + " WHEN p.proparallel = " + CppAsString2(PROPARALLEL_RESTRICTED) " THEN '%s'\n" + " WHEN p.proparallel = " + CppAsString2(PROPARALLEL_SAFE) " THEN '%s'\n" + " WHEN p.proparallel = " + CppAsString2(PROPARALLEL_UNSAFE) " THEN '%s'\n" " END as \"%s\"", gettext_noop("restricted"), gettext_noop("safe"), @@ -448,7 +460,8 @@ describeFunctions(const char *functypes, const char *func_pattern, have_where = true; } if (pset.sversion >= 110000) - appendPQExpBufferStr(&buf, "p.prokind <> 'a'\n"); + appendPQExpBufferStr(&buf, "p.prokind <> " + CppAsString2(PROKIND_AGGREGATE) "\n"); else appendPQExpBufferStr(&buf, "NOT p.proisagg\n"); } @@ -461,7 +474,8 @@ describeFunctions(const char *functypes, const char *func_pattern, appendPQExpBufferStr(&buf, "WHERE "); have_where = true; } - appendPQExpBufferStr(&buf, "p.prokind <> 'p'\n"); + appendPQExpBufferStr(&buf, "p.prokind <> " + CppAsString2(PROKIND_PROCEDURE) "\n"); } if (!showTrigger) { @@ -484,7 +498,8 @@ describeFunctions(const char *functypes, const char *func_pattern, have_where = true; } if (pset.sversion >= 110000) - appendPQExpBufferStr(&buf, "p.prokind <> 'w'\n"); + appendPQExpBufferStr(&buf, "p.prokind <> " + CppAsString2(PROKIND_WINDOW) "\n"); else appendPQExpBufferStr(&buf, "NOT p.proiswindow\n"); } @@ -499,7 +514,8 @@ describeFunctions(const char *functypes, const char *func_pattern, if (showAggregate) { if (pset.sversion >= 110000) - appendPQExpBufferStr(&buf, "p.prokind = 'a'\n"); + appendPQExpBufferStr(&buf, "p.prokind = " + CppAsString2(PROKIND_AGGREGATE) "\n"); else appendPQExpBufferStr(&buf, "p.proisagg\n"); needs_or = true; @@ -516,7 +532,7 @@ describeFunctions(const char *functypes, const char *func_pattern, { if (needs_or) appendPQExpBufferStr(&buf, " OR "); - appendPQExpBufferStr(&buf, "p.prokind = 'p'\n"); + appendPQExpBufferStr(&buf, "p.prokind = " CppAsString2(PROKIND_PROCEDURE) "\n"); needs_or = true; } if (showWindow) @@ -524,7 +540,8 @@ describeFunctions(const char *functypes, const char *func_pattern, if (needs_or) appendPQExpBufferStr(&buf, " OR "); if (pset.sversion >= 110000) - appendPQExpBufferStr(&buf, "p.prokind = 'w'\n"); + appendPQExpBufferStr(&buf, "p.prokind = " + CppAsString2(PROKIND_WINDOW) "\n"); else appendPQExpBufferStr(&buf, "p.proiswindow\n"); } @@ -926,7 +943,11 @@ listAllDbs(const char *pattern, bool verbose) gettext_noop("Encoding")); if (pset.sversion >= 150000) appendPQExpBuffer(&buf, - " CASE d.datlocprovider WHEN 'b' THEN 'builtin' WHEN 'c' THEN 'libc' WHEN 'i' THEN 'icu' END AS \"%s\",\n", + " CASE d.datlocprovider " + "WHEN " CppAsString2(COLLPROVIDER_BUILTIN) " THEN 'builtin' " + "WHEN " CppAsString2(COLLPROVIDER_LIBC) " THEN 'libc' " + "WHEN " CppAsString2(COLLPROVIDER_ICU) " THEN 'icu' " + "END AS \"%s\",\n", gettext_noop("Locale Provider")); else appendPQExpBuffer(&buf, @@ -1812,7 +1833,7 @@ describeOneTableDetails(const char *schemaname, } PQclear(result); - if (tableinfo.relpersistence == 'u') + if (tableinfo.relpersistence == RELPERSISTENCE_UNLOGGED) printfPQExpBuffer(&title, _("Unlogged sequence \"%s.%s\""), schemaname, relationname); else @@ -1958,7 +1979,7 @@ describeOneTableDetails(const char *schemaname, switch (tableinfo.relkind) { case RELKIND_RELATION: - if (tableinfo.relpersistence == 'u') + if (tableinfo.relpersistence == RELPERSISTENCE_UNLOGGED) printfPQExpBuffer(&title, _("Unlogged table \"%s.%s\""), schemaname, relationname); else @@ -1974,7 +1995,7 @@ describeOneTableDetails(const char *schemaname, schemaname, relationname); break; case RELKIND_INDEX: - if (tableinfo.relpersistence == 'u') + if (tableinfo.relpersistence == RELPERSISTENCE_UNLOGGED) printfPQExpBuffer(&title, _("Unlogged index \"%s.%s\""), schemaname, relationname); else @@ -1982,7 +2003,7 @@ describeOneTableDetails(const char *schemaname, schemaname, relationname); break; case RELKIND_PARTITIONED_INDEX: - if (tableinfo.relpersistence == 'u') + if (tableinfo.relpersistence == RELPERSISTENCE_UNLOGGED) printfPQExpBuffer(&title, _("Unlogged partitioned index \"%s.%s\""), schemaname, relationname); else @@ -2002,7 +2023,7 @@ describeOneTableDetails(const char *schemaname, schemaname, relationname); break; case RELKIND_PARTITIONED_TABLE: - if (tableinfo.relpersistence == 'u') + if (tableinfo.relpersistence == RELPERSISTENCE_UNLOGGED) printfPQExpBuffer(&title, _("Unlogged partitioned table \"%s.%s\""), schemaname, relationname); else @@ -2107,10 +2128,10 @@ describeOneTableDetails(const char *schemaname, char *storage = PQgetvalue(res, i, attstorage_col); /* these strings are literal in our syntax, so not translated. */ - printTableAddCell(&cont, (storage[0] == 'p' ? "plain" : - (storage[0] == 'm' ? "main" : - (storage[0] == 'x' ? "extended" : - (storage[0] == 'e' ? "external" : + printTableAddCell(&cont, (storage[0] == TYPSTORAGE_PLAIN ? "plain" : + (storage[0] == TYPSTORAGE_MAIN ? "main" : + (storage[0] == TYPSTORAGE_EXTENDED ? "extended" : + (storage[0] == TYPSTORAGE_EXTERNAL ? "external" : "???")))), false, false); } @@ -2258,13 +2279,17 @@ describeOneTableDetails(const char *schemaname, "EXISTS (SELECT 1 FROM pg_catalog.pg_constraint " "WHERE conrelid = i.indrelid AND " "conindid = i.indexrelid AND " - "contype IN ('p','u','x') AND " + "contype IN (" CppAsString2(CONSTRAINT_PRIMARY) "," + CppAsString2(CONSTRAINT_UNIQUE) "," + CppAsString2(CONSTRAINT_EXCLUSION) ") AND " "condeferrable) AS condeferrable,\n" " (NOT i.indimmediate) AND " "EXISTS (SELECT 1 FROM pg_catalog.pg_constraint " "WHERE conrelid = i.indrelid AND " "conindid = i.indexrelid AND " - "contype IN ('p','u','x') AND " + "contype IN (" CppAsString2(CONSTRAINT_PRIMARY) "," + CppAsString2(CONSTRAINT_UNIQUE) "," + CppAsString2(CONSTRAINT_EXCLUSION) ") AND " "condeferred) AS condeferred,\n"); if (pset.sversion >= 90400) @@ -2385,7 +2410,10 @@ describeOneTableDetails(const char *schemaname, appendPQExpBufferStr(&buf, ", false AS conperiod"); appendPQExpBuffer(&buf, "\nFROM pg_catalog.pg_class c, pg_catalog.pg_class c2, pg_catalog.pg_index i\n" - " LEFT JOIN pg_catalog.pg_constraint con ON (conrelid = i.indrelid AND conindid = i.indexrelid AND contype IN ('p','u','x'))\n" + " LEFT JOIN pg_catalog.pg_constraint con ON (conrelid = i.indrelid AND conindid = i.indexrelid AND contype IN (" + CppAsString2(CONSTRAINT_PRIMARY) "," + CppAsString2(CONSTRAINT_UNIQUE) "," + CppAsString2(CONSTRAINT_EXCLUSION) "))\n" "WHERE c.oid = '%s' AND c.oid = i.indrelid AND i.indexrelid = c2.oid\n" "ORDER BY i.indisprimary DESC, c2.relname;", oid); @@ -2473,7 +2501,8 @@ describeOneTableDetails(const char *schemaname, "SELECT r.conname, " "pg_catalog.pg_get_constraintdef(r.oid, true)\n" "FROM pg_catalog.pg_constraint r\n" - "WHERE r.conrelid = '%s' AND r.contype = 'c'\n" + "WHERE r.conrelid = '%s' " + "AND r.contype = " CppAsString2(CONSTRAINT_CHECK) "\n" "ORDER BY 1;", oid); result = PSQLexec(buf.data); @@ -2520,7 +2549,7 @@ describeOneTableDetails(const char *schemaname, " conrelid::pg_catalog.regclass AS ontable\n" " FROM pg_catalog.pg_constraint,\n" " pg_catalog.pg_partition_ancestors('%s')\n" - " WHERE conrelid = relid AND contype = 'f' AND conparentid = 0\n" + " WHERE conrelid = relid AND contype = " CppAsString2(CONSTRAINT_FOREIGN) " AND conparentid = 0\n" "ORDER BY sametable DESC, conname;", oid, oid); } @@ -2531,7 +2560,7 @@ describeOneTableDetails(const char *schemaname, " pg_catalog.pg_get_constraintdef(r.oid, true) as condef,\n" " conrelid::pg_catalog.regclass AS ontable\n" "FROM pg_catalog.pg_constraint r\n" - "WHERE r.conrelid = '%s' AND r.contype = 'f'\n", + "WHERE r.conrelid = '%s' AND r.contype = " CppAsString2(CONSTRAINT_FOREIGN) "\n", oid); if (pset.sversion >= 120000) @@ -2588,7 +2617,7 @@ describeOneTableDetails(const char *schemaname, " FROM pg_catalog.pg_constraint c\n" " WHERE confrelid IN (SELECT pg_catalog.pg_partition_ancestors('%s')\n" " UNION ALL VALUES ('%s'::pg_catalog.regclass))\n" - " AND contype = 'f' AND conparentid = 0\n" + " AND contype = " CppAsString2(CONSTRAINT_FOREIGN) " AND conparentid = 0\n" "ORDER BY conname;", oid, oid); } @@ -2598,7 +2627,7 @@ describeOneTableDetails(const char *schemaname, "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" - " WHERE confrelid = %s AND contype = 'f'\n" + " WHERE confrelid = %s AND contype = " CppAsString2(CONSTRAINT_FOREIGN) "\n" "ORDER BY conname;", oid); } @@ -2720,9 +2749,9 @@ describeOneTableDetails(const char *schemaname, "stxnamespace::pg_catalog.regnamespace::pg_catalog.text AS nsp, " "stxname,\n" "pg_catalog.pg_get_statisticsobjdef_columns(oid) AS columns,\n" - " 'd' = any(stxkind) AS ndist_enabled,\n" - " 'f' = any(stxkind) AS deps_enabled,\n" - " 'm' = any(stxkind) AS mcv_enabled,\n" + " " CppAsString2(STATS_EXT_NDISTINCT) " = any(stxkind) AS ndist_enabled,\n" + " " CppAsString2(STATS_EXT_DEPENDENCIES) " = any(stxkind) AS deps_enabled,\n" + " " CppAsString2(STATS_EXT_MCV) " = any(stxkind) AS mcv_enabled,\n" "stxstattarget\n" "FROM pg_catalog.pg_statistic_ext\n" "WHERE stxrelid = '%s'\n" @@ -2821,9 +2850,9 @@ describeOneTableDetails(const char *schemaname, " FROM pg_catalog.unnest(stxkeys) s(attnum)\n" " JOIN pg_catalog.pg_attribute a ON (stxrelid = a.attrelid AND\n" " a.attnum = s.attnum AND NOT attisdropped)) AS columns,\n" - " 'd' = any(stxkind) AS ndist_enabled,\n" - " 'f' = any(stxkind) AS deps_enabled,\n" - " 'm' = any(stxkind) AS mcv_enabled,\n"); + " " CppAsString2(STATS_EXT_NDISTINCT) " = any(stxkind) AS ndist_enabled,\n" + " " CppAsString2(STATS_EXT_DEPENDENCIES) " = any(stxkind) AS deps_enabled,\n" + " " CppAsString2(STATS_EXT_MCV) " = any(stxkind) AS mcv_enabled,\n"); if (pset.sversion >= 130000) appendPQExpBufferStr(&buf, " stxstattarget\n"); @@ -3483,16 +3512,18 @@ describeOneTableDetails(const char *schemaname, * No need to display default values; we already display a REPLICA * IDENTITY marker on indexes. */ - tableinfo.relreplident != 'i' && - ((strcmp(schemaname, "pg_catalog") != 0 && tableinfo.relreplident != 'd') || - (strcmp(schemaname, "pg_catalog") == 0 && tableinfo.relreplident != 'n'))) + tableinfo.relreplident != REPLICA_IDENTITY_INDEX && + ((strcmp(schemaname, "pg_catalog") != 0 && + tableinfo.relreplident != REPLICA_IDENTITY_DEFAULT) || + (strcmp(schemaname, "pg_catalog") == 0 && + tableinfo.relreplident != REPLICA_IDENTITY_NOTHING))) { const char *s = _("Replica Identity"); printfPQExpBuffer(&buf, "%s: %s", s, - tableinfo.relreplident == 'f' ? "FULL" : - tableinfo.relreplident == 'n' ? "NOTHING" : + tableinfo.relreplident == REPLICA_IDENTITY_FULL ? "FULL" : + tableinfo.relreplident == REPLICA_IDENTITY_DEFAULT ? "NOTHING" : "???"); printTableAddFooter(&cont, buf.data); @@ -3975,7 +4006,11 @@ listTables(const char *tabtypes, const char *pattern, bool verbose, bool showSys * Show whether a relation is permanent, temporary, or unlogged. */ appendPQExpBuffer(&buf, - ",\n CASE c.relpersistence WHEN 'p' THEN '%s' WHEN 't' THEN '%s' WHEN 'u' THEN '%s' END as \"%s\"", + ",\n CASE c.relpersistence " + "WHEN " CppAsString2(RELPERSISTENCE_PERMANENT) " THEN '%s' " + "WHEN " CppAsString2(RELPERSISTENCE_TEMP) " THEN '%s' " + "WHEN " CppAsString2(RELPERSISTENCE_UNLOGGED) " THEN '%s' " + "END as \"%s\"", gettext_noop("permanent"), gettext_noop("temporary"), gettext_noop("unlogged"), @@ -4411,7 +4446,7 @@ listDomains(const char *pattern, bool verbose, bool showSystem) " CASE WHEN t.typnotnull THEN 'not null' END as \"%s\",\n" " t.typdefault as \"%s\",\n" " pg_catalog.array_to_string(ARRAY(\n" - " SELECT pg_catalog.pg_get_constraintdef(r.oid, true) FROM pg_catalog.pg_constraint r WHERE t.oid = r.contypid AND r.contype = 'c' ORDER BY r.conname\n" + " SELECT pg_catalog.pg_get_constraintdef(r.oid, true) FROM pg_catalog.pg_constraint r WHERE t.oid = r.contypid AND r.contype = " CppAsString2(CONSTRAINT_CHECK) " ORDER BY r.conname\n" " ), ' ') as \"%s\"", gettext_noop("Schema"), gettext_noop("Name"), @@ -4748,9 +4783,10 @@ listExtendedStats(const char *pattern) gettext_noop("Definition")); appendPQExpBuffer(&buf, - ",\nCASE WHEN 'd' = any(es.stxkind) THEN 'defined' \n" + ",\nCASE " + "WHEN " CppAsString2(STATS_EXT_NDISTINCT) " = any(es.stxkind) THEN 'defined' \n" "END AS \"%s\", \n" - "CASE WHEN 'f' = any(es.stxkind) THEN 'defined' \n" + "CASE WHEN " CppAsString2(STATS_EXT_DEPENDENCIES) " = any(es.stxkind) THEN 'defined' \n" "END AS \"%s\"", gettext_noop("Ndistinct"), gettext_noop("Dependencies")); @@ -4761,7 +4797,7 @@ listExtendedStats(const char *pattern) if (pset.sversion >= 120000) { appendPQExpBuffer(&buf, - ",\nCASE WHEN 'm' = any(es.stxkind) THEN 'defined' \n" + ",\nCASE WHEN " CppAsString2(STATS_EXT_MCV) " = any(es.stxkind) THEN 'defined' \n" "END AS \"%s\" ", gettext_noop("MCV")); } @@ -4937,7 +4973,12 @@ listCollations(const char *pattern, bool verbose, bool showSystem) if (pset.sversion >= 100000) appendPQExpBuffer(&buf, - " CASE c.collprovider WHEN 'd' THEN 'default' WHEN 'b' THEN 'builtin' WHEN 'c' THEN 'libc' WHEN 'i' THEN 'icu' END AS \"%s\",\n", + " CASE c.collprovider " + "WHEN " CppAsString2(COLLPROVIDER_DEFAULT) " THEN 'default' " + "WHEN " CppAsString2(COLLPROVIDER_BUILTIN) " THEN 'builtin' " + "WHEN " CppAsString2(COLLPROVIDER_LIBC) " THEN 'libc' " + "WHEN " CppAsString2(COLLPROVIDER_ICU) " THEN 'icu' " + "END AS \"%s\",\n", gettext_noop("Provider")); else appendPQExpBuffer(&buf, @@ -6901,8 +6942,8 @@ listOpFamilyOperators(const char *access_method_pattern, " o.amopopr::pg_catalog.regoperator AS \"%s\"\n," " o.amopstrategy AS \"%s\",\n" " CASE o.amoppurpose\n" - " WHEN 'o' THEN '%s'\n" - " WHEN 's' THEN '%s'\n" + " WHEN " CppAsString2(AMOP_ORDER) " THEN '%s'\n" + " WHEN " CppAsString2(AMOP_SEARCH) " THEN '%s'\n" " END AS \"%s\"\n", gettext_noop("AM"), gettext_noop("Operator family"), -- 2.45.2
signature.asc
Description: PGP signature