Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1n4qdL-00009R-A9 for pgsql-hackers@arkaria.postgresql.org; Tue, 04 Jan 2022 20:42:15 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1n4qdJ-0006IC-CN for pgsql-hackers@arkaria.postgresql.org; Tue, 04 Jan 2022 20:42:13 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1n4qdJ-0006I3-1Y for pgsql-hackers@lists.postgresql.org; Tue, 04 Jan 2022 20:42:13 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1n4qdG-00011x-Dj for pgsql-hackers@lists.postgresql.org; Tue, 04 Jan 2022 20:42:12 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 204Kg7kb4116007; Tue, 4 Jan 2022 15:42:07 -0500 From: Tom Lane To: Justin Pryzby cc: Pavel Luzanov , pgsql-hackers@lists.postgresql.org Subject: Re: psql: \dl+ to list large objects privileges In-reply-to: <20220104062417.GA14051@telsasoft.com> References: <6d722115-6297-bc53-bb7f-5f150e765299@postgrespro.ru> <20220104062417.GA14051@telsasoft.com> Comments: In-reply-to Justin Pryzby message dated "Tue, 04 Jan 2022 00:24:18 -0600" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <4115968.1641328886.0@sss.pgh.pa.us> Date: Tue, 04 Jan 2022 15:42:07 -0500 Message-ID: <4116006.1641328927@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <4115968.1641328886.1@sss.pgh.pa.us> Content-Transfer-Encoding: quoted-printable Justin Pryzby 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 i= t > 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="0001-move-and-rename-do_lo_list.patch"; charset="us-ascii" Content-ID: <4115968.1641328886.2@sss.pgh.pa.us> Content-Description: 0001-move-and-rename-do_lo_list.patch Content-Transfer-Encoding: quoted-printable 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_b= ranch, const char *cmd) success =3D describeRoles(pattern, show_verbose, show_system); break; case 'l': - success =3D do_lo_list(); + success =3D listLargeObjects(); break; case 'L': success =3D listLanguages(pattern, show_verbose, show_system); @@ -1963,7 +1963,7 @@ exec_command_lo(PsqlScanState scan_state, bool activ= e_branch, const char *cmd) } = else if (strcmp(cmd + 3, "list") =3D=3D 0) - success =3D do_lo_list(); + success =3D listLargeObjects(); = else if (strcmp(cmd + 3, "unlink") =3D=3D 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_pat= tern, PQclear(res); return true; } + +/* + * \dl or \lo_list + * Lists large objects + */ +bool +listLargeObjects(void) +{ + PGresult *res; + char buf[1024]; + printQueryOpt myopt =3D 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 =3D PSQLexec(buf); + if (!res) + return false; + + myopt.topt.tuples_only =3D false; + myopt.nullPrint =3D NULL; + myopt.title =3D _("Large objects"); + myopt.translate_header =3D 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 *accessMe= thod_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 =3D 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 =3D PSQLexec(buf); - if (!res) - return false; - - myopt.topt.tuples_only =3D false; - myopt.nullPrint =3D NULL; - myopt.title =3D _("Large objects"); - myopt.translate_header =3D 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 */ ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="0002-print-large-object-acls.patch"; charset="us-ascii" Content-ID: <4115968.1641328886.3@sss.pgh.pa.us> Content-Description: 0002-print-large-object-acls.patch Content-Transfer-Encoding: quoted-printable 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; LARGE OBJECT rw none - + \dl+ SCHEMA diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sg= ml 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=3D> = = - \dl + \dl[+] This is an alias for \lo_list, which shows a list of large objects. + If + is appended to the command name, + each large object is listed with its associated permissions, + if any. @@ -2610,12 +2613,15 @@ lo_import 152801 = - \lo_list + \lo_list[+] Shows a list of all PostgreSQL large objects currently stored in the database, along with any comments provided for them. + If + is appended to the command name, + each large object is listed with its associated permissions, + if any. 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 =3D describeRoles(pattern, show_verbose, show_system); break; case 'l': - success =3D listLargeObjects(); + switch (cmd[2]) + { + case '\0': + case '+': + success =3D listLargeObjects(show_verbose); + break; + default: + status =3D PSQL_CMD_UNKNOWN; + break; + } break; case 'L': success =3D listLanguages(pattern, show_verbose, show_system); @@ -1928,11 +1937,13 @@ exec_command_lo(PsqlScanState scan_state, bool act= ive_branch, const char *cmd) { char *opt1, *opt2; + bool show_verbose; = opt1 =3D psql_scan_slash_option(scan_state, OT_NORMAL, NULL, true); opt2 =3D psql_scan_slash_option(scan_state, OT_NORMAL, NULL, true); + show_verbose =3D strchr(cmd, '+') ? true : false; = if (strcmp(cmd + 3, "export") =3D=3D 0) { @@ -1962,8 +1973,8 @@ exec_command_lo(PsqlScanState scan_state, bool activ= e_branch, const char *cmd) } } = - else if (strcmp(cmd + 3, "list") =3D=3D 0) - success =3D listLargeObjects(); + else if (strcmp(cmd + 3, "list") =3D=3D 0 || strcmp(cmd + 3, "list+") =3D= =3D 0) + success =3D listLargeObjects(show_verbose); = else if (strcmp(cmd + 3, "unlink") =3D=3D 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_pa= ttern, * Lists large objects */ bool -listLargeObjects(void) +listLargeObjects(bool verbose) { + PQExpBufferData buf; PGresult *res; - char buf[1024]; printQueryOpt myopt =3D 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 =3D 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 =3D PSQLexec(buf.data); + termPQExpBuffer(&buf); if (!res) return false; = - myopt.topt.tuples_only =3D false; myopt.nullPrint =3D NULL; myopt.title =3D _("Large objects"); myopt.translate_header =3D 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_m= ethod_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/expecte= d/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=3Drw/lo_test+| the answer to the ultimate questio= n of life + | | =3Dr/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=3Drw/lo_test+| the answer to the ultimate questio= n of life + | | =3Dr/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=3D42 and owner lo_test is expected in the ou= tput +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 lif= e'; +GRANT SELECT ON LARGE OBJECT 42 TO public; +\dl +\dl+ +\lo_list +\lo_list+ +\dl- +\lo_list- +\lo_unlink 42 +DROP ROLE lo_test; ------- =_aaaaaaaaaa0--