Re: psql: \dl+ to list large objects privileges - Mailing list pgsql-hackers

Justin Pryzby <pryzby@telsasoft.com> writes:
> I suggest to move the function in a separate 0001 commit, which makes no code
> changes other than moving from one file to another.
> A committer would probably push them as a single patch, but this makes it
> easier to read and review the changes in 0002.

Yeah, I agree with that idea.  It's really tough to notice small changes
by hand when the entire code block has been moved somewhere else.

> Since a few weeks ago, psql no longer supports server versions before 9.2, so
> the "if" branch can go away.

And, in fact, the patch is no longer applying per the cfbot, because
that hasn't been done.

To move things along a bit, I split the patch more or less as Justin
suggests and brought it up to HEAD.  I did not study the command.c
changes, but the rest of it seems okay, with one exception: I don't like
the test case much at all.  For one thing, it will fail in the buildfarm
because you didn't adhere to the convention that roles created by a
regression test must be named regress_something.  For another, there's
considerable overlap with testing done in largeobject.sql, which
already creates some commented large objects.  That means there's
an undesirable ordering dependency --- if somebody wanted to run
largeobject.sql first, the expected output of psql.sql would change.
I wonder if we shouldn't put these new tests in largeobject.sql instead.
(That would mean there are two expected-files to change, which is a bit
tedious, but it's not very hard.)

            regards, tom lane

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index fb3bab9494..0d64757899 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -811,7 +811,7 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
                 success = describeRoles(pattern, show_verbose, show_system);
                 break;
             case 'l':
-                success = do_lo_list();
+                success = listLargeObjects();
                 break;
             case 'L':
                 success = listLanguages(pattern, show_verbose, show_system);
@@ -1963,7 +1963,7 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
         }

         else if (strcmp(cmd + 3, "list") == 0)
-            success = do_lo_list();
+            success = listLargeObjects();

         else if (strcmp(cmd + 3, "unlink") == 0)
         {
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index c28788e84f..0d92f3a80b 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6455,3 +6455,39 @@ listOpFamilyFunctions(const char *access_method_pattern,
     PQclear(res);
     return true;
 }
+
+/*
+ * \dl or \lo_list
+ * Lists large objects
+ */
+bool
+listLargeObjects(void)
+{
+    PGresult   *res;
+    char        buf[1024];
+    printQueryOpt myopt = pset.popt;
+
+    snprintf(buf, sizeof(buf),
+             "SELECT oid as \"%s\",\n"
+             "  pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n"
+             "  pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
+             "  FROM pg_catalog.pg_largeobject_metadata "
+             "  ORDER BY oid",
+             gettext_noop("ID"),
+             gettext_noop("Owner"),
+             gettext_noop("Description"));
+
+    res = PSQLexec(buf);
+    if (!res)
+        return false;
+
+    myopt.topt.tuples_only = false;
+    myopt.nullPrint = NULL;
+    myopt.title = _("Large objects");
+    myopt.translate_header = true;
+
+    printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
+
+    PQclear(res);
+    return true;
+}
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index 71b320f1fc..3a55e1e4ed 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -139,5 +139,7 @@ extern bool listOpFamilyOperators(const char *accessMethod_pattern,
 extern bool listOpFamilyFunctions(const char *access_method_pattern,
                                   const char *family_pattern, bool verbose);

+/* \dl or \lo_list */
+extern bool listLargeObjects(void);

 #endif                            /* DESCRIBE_H */
diff --git a/src/bin/psql/large_obj.c b/src/bin/psql/large_obj.c
index 10e47c87ac..243875be83 100644
--- a/src/bin/psql/large_obj.c
+++ b/src/bin/psql/large_obj.c
@@ -262,42 +262,3 @@ do_lo_unlink(const char *loid_arg)

     return true;
 }
-
-
-
-/*
- * do_lo_list()
- *
- * Show all large objects in database with comments
- */
-bool
-do_lo_list(void)
-{
-    PGresult   *res;
-    char        buf[1024];
-    printQueryOpt myopt = pset.popt;
-
-    snprintf(buf, sizeof(buf),
-             "SELECT oid as \"%s\",\n"
-             "  pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n"
-             "  pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
-             "  FROM pg_catalog.pg_largeobject_metadata "
-             "  ORDER BY oid",
-             gettext_noop("ID"),
-             gettext_noop("Owner"),
-             gettext_noop("Description"));
-
-    res = PSQLexec(buf);
-    if (!res)
-        return false;
-
-    myopt.topt.tuples_only = false;
-    myopt.nullPrint = NULL;
-    myopt.title = _("Large objects");
-    myopt.translate_header = true;
-
-    printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
-
-    PQclear(res);
-    return true;
-}
diff --git a/src/bin/psql/large_obj.h b/src/bin/psql/large_obj.h
index 003acbf52c..3172a7704d 100644
--- a/src/bin/psql/large_obj.h
+++ b/src/bin/psql/large_obj.h
@@ -11,6 +11,5 @@
 bool        do_lo_export(const char *loid_arg, const char *filename_arg);
 bool        do_lo_import(const char *filename_arg, const char *comment_arg);
 bool        do_lo_unlink(const char *loid_arg);
-bool        do_lo_list(void);

 #endif                            /* LARGE_OBJ_H */
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 64d9030652..22f6c5c7ab 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2146,7 +2146,7 @@ REVOKE ALL ON accounts FROM PUBLIC;
       <entry><literal>LARGE OBJECT</literal></entry>
       <entry><literal>rw</literal></entry>
       <entry>none</entry>
-      <entry></entry>
+      <entry><literal>\dl+</literal></entry>
      </row>
      <row>
       <entry><literal>SCHEMA</literal></entry>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae38d3dcc3..1ab200a4ad 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1681,11 +1681,14 @@ testdb=>


       <varlistentry>
-        <term><literal>\dl</literal></term>
+        <term><literal>\dl[+]</literal></term>
         <listitem>
         <para>
         This is an alias for <command>\lo_list</command>, which shows a
         list of large objects.
+        If <literal>+</literal> is appended to the command name,
+        each large object is listed with its associated permissions,
+        if any.
         </para>
         </listitem>
       </varlistentry>
@@ -2610,12 +2613,15 @@ lo_import 152801
       </varlistentry>

       <varlistentry>
-        <term><literal>\lo_list</literal></term>
+        <term><literal>\lo_list[+]</literal></term>
         <listitem>
         <para>
         Shows a list of all <productname>PostgreSQL</productname>
         large objects currently stored in the database,
         along with any comments provided for them.
+        If <literal>+</literal> is appended to the command name,
+        each large object is listed with its associated permissions,
+        if any.
         </para>
         </listitem>
       </varlistentry>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 0d64757899..ceedcc860c 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -811,7 +811,16 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
                 success = describeRoles(pattern, show_verbose, show_system);
                 break;
             case 'l':
-                success = listLargeObjects();
+                switch (cmd[2])
+                {
+                    case '\0':
+                    case '+':
+                        success = listLargeObjects(show_verbose);
+                        break;
+                    default:
+                        status = PSQL_CMD_UNKNOWN;
+                        break;
+                }
                 break;
             case 'L':
                 success = listLanguages(pattern, show_verbose, show_system);
@@ -1928,11 +1937,13 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
     {
         char       *opt1,
                    *opt2;
+        bool        show_verbose;

         opt1 = psql_scan_slash_option(scan_state,
                                       OT_NORMAL, NULL, true);
         opt2 = psql_scan_slash_option(scan_state,
                                       OT_NORMAL, NULL, true);
+        show_verbose = strchr(cmd, '+') ? true : false;

         if (strcmp(cmd + 3, "export") == 0)
         {
@@ -1962,8 +1973,8 @@ exec_command_lo(PsqlScanState scan_state, bool active_branch, const char *cmd)
             }
         }

-        else if (strcmp(cmd + 3, "list") == 0)
-            success = listLargeObjects();
+        else if (strcmp(cmd + 3, "list") == 0 || strcmp(cmd + 3, "list+") == 0)
+            success = listLargeObjects(show_verbose);

         else if (strcmp(cmd + 3, "unlink") == 0)
         {
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 0d92f3a80b..5edab7f938 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6461,27 +6461,37 @@ listOpFamilyFunctions(const char *access_method_pattern,
  * Lists large objects
  */
 bool
-listLargeObjects(void)
+listLargeObjects(bool verbose)
 {
+    PQExpBufferData buf;
     PGresult   *res;
-    char        buf[1024];
     printQueryOpt myopt = pset.popt;

-    snprintf(buf, sizeof(buf),
-             "SELECT oid as \"%s\",\n"
-             "  pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n"
-             "  pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
-             "  FROM pg_catalog.pg_largeobject_metadata "
-             "  ORDER BY oid",
-             gettext_noop("ID"),
-             gettext_noop("Owner"),
-             gettext_noop("Description"));
-
-    res = PSQLexec(buf);
+    initPQExpBuffer(&buf);
+
+    printfPQExpBuffer(&buf,
+                      "SELECT oid as \"%s\",\n"
+                      "  pg_catalog.pg_get_userbyid(lomowner) as \"%s\",\n  ",
+                      gettext_noop("ID"),
+                      gettext_noop("Owner"));
+
+    if (verbose)
+    {
+        printACLColumn(&buf, "lomacl");
+        appendPQExpBufferStr(&buf, ",\n  ");
+    }
+
+    appendPQExpBuffer(&buf,
+                      "pg_catalog.obj_description(oid, 'pg_largeobject') as \"%s\"\n"
+                      "FROM pg_catalog.pg_largeobject_metadata\n"
+                      "ORDER BY oid",
+                      gettext_noop("Description"));
+
+    res = PSQLexec(buf.data);
+    termPQExpBuffer(&buf);
     if (!res)
         return false;

-    myopt.topt.tuples_only = false;
     myopt.nullPrint = NULL;
     myopt.title = _("Large objects");
     myopt.translate_header = true;
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index 3a55e1e4ed..b57ba67bba 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -140,6 +140,6 @@ extern bool listOpFamilyFunctions(const char *access_method_pattern,
                                   const char *family_pattern, bool verbose);

 /* \dl or \lo_list */
-extern bool listLargeObjects(void);
+extern bool listLargeObjects(bool verbose);

 #endif                            /* DESCRIBE_H */
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 8cadfbb103..5752a36ac8 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -248,7 +248,7 @@ slashUsage(unsigned short int pager)
     fprintf(output, _("  \\dFt[+] [PATTERN]      list text search templates\n"));
     fprintf(output, _("  \\dg[S+] [PATTERN]      list roles\n"));
     fprintf(output, _("  \\di[S+] [PATTERN]      list indexes\n"));
-    fprintf(output, _("  \\dl                    list large objects, same as \\lo_list\n"));
+    fprintf(output, _("  \\dl[+]                 list large objects, same as \\lo_list\n"));
     fprintf(output, _("  \\dL[S+] [PATTERN]      list procedural languages\n"));
     fprintf(output, _("  \\dm[S+] [PATTERN]      list materialized views\n"));
     fprintf(output, _("  \\dn[S+] [PATTERN]      list schemas\n"));
@@ -325,7 +325,7 @@ slashUsage(unsigned short int pager)
     fprintf(output, _("Large Objects\n"));
     fprintf(output, _("  \\lo_export LOBOID FILE\n"
                       "  \\lo_import FILE [COMMENT]\n"
-                      "  \\lo_list\n"
+                      "  \\lo_list[+]\n"
                       "  \\lo_unlink LOBOID      large object operations\n"));

     ClosePager(output);
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 6428ebc507..0aa98317a7 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -5290,3 +5290,36 @@ ERROR:  relation "notexists" does not exist
 LINE 1: SELECT * FROM notexists;
                       ^
 STATEMENT:  SELECT * FROM notexists;
+ lo_create
+-----------
+        42
+(1 row)
+
+                       Large objects
+ ID |  Owner  |                 Description
+----+---------+---------------------------------------------
+ 42 | lo_test | the answer to the ultimate question of life
+(1 row)
+
+                                  Large objects
+ ID |  Owner  | Access privileges  |                 Description
+----+---------+--------------------+---------------------------------------------
+ 42 | lo_test | lo_test=rw/lo_test+| the answer to the ultimate question of life
+    |         | =r/lo_test         |
+(1 row)
+
+                       Large objects
+ ID |  Owner  |                 Description
+----+---------+---------------------------------------------
+ 42 | lo_test | the answer to the ultimate question of life
+(1 row)
+
+                                  Large objects
+ ID |  Owner  | Access privileges  |                 Description
+----+---------+--------------------+---------------------------------------------
+ 42 | lo_test | lo_test=rw/lo_test+| the answer to the ultimate question of life
+    |         | =r/lo_test         |
+(1 row)
+
+invalid command \dl-
+invalid command \lo_list-
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index d4e4fdbbb7..dea081f937 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1316,3 +1316,19 @@ DROP TABLE oer_test;
 \set ECHO errors
 SELECT * FROM notexists;
 \set ECHO none
+
+-- check printing info about large objects
+-- one large object with OID=42 and owner lo_test is expected in the output
+CREATE ROLE lo_test;
+SELECT lo_create(42);
+ALTER LARGE OBJECT 42 OWNER TO lo_test;
+COMMENT ON LARGE OBJECT 42 IS 'the answer to the ultimate question of life';
+GRANT SELECT ON LARGE OBJECT 42 TO public;
+\dl
+\dl+
+\lo_list
+\lo_list+
+\dl-
+\lo_list-
+\lo_unlink 42
+DROP ROLE lo_test;