Re: [PATCHES] [HACKERS] Show INHERIT in \du
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On Sun, Apr 13, 2008 at 6:11 PM, Brendan Jurd wrote: I've written a patch which implements the same \du behaviour as my previous patch, but using the new printTable API New versions of this patch, including changes made to the printTable API in [1] As before, I've included a patch against HEAD which includes the printTable changes (du-attributes-print-table_2.diff.bz2), and a patch against my printTable branch which shows just the changes to describeRoles (du-attributes_2.diff.bz2). Cheers, BJ [1] http://archives.postgresql.org/message-id/[EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.7 (GNU/Linux) Comment: http://getfiregpg.org iD8DBQFIBGZP5YBsbHkuyV0RAsb9AKDrFJ/8V00t2XCwIihzEZYcPQZKiQCg3q6L RkiMfjqLay/JLk8phnniYLs= =oW6S -END PGP SIGNATURE- du-attributes-print-table_2.diff.bz2 Description: BZip2 compressed data du-attributes_2.diff.bz2 Description: BZip2 compressed data -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
On Tue, Mar 25, 2008 at 2:41 AM, Tom Lane [EMAIL PROTECTED] wrote: Brendan Jurd [EMAIL PROTECTED] writes: This makes me wonder whether print.c could offer something a bit more helpful to callers wishing to DIY a table; we could have a table-building struct with methods like addHeader and addCell. What do you think? Overkill, or worthy pursuit? Once you have two occurrences of a pattern, it's reasonable to assume there will be more later. +1 for building a little bit of infrastructure. I've written a patch which implements the same \du behaviour as my previous patch, but using the new printTable API I submitted in [1]. If the printTable API patch is rejected or substantially changed, we will need to revisit this patch. The new patch constructs a table manually, in the same manner as describeOneTableDetails, so that we get the same outputs as the original patch but without any of the localisation issues identified by Tom and Alvaro. I have attached a patch against my printTable code, containing only the changes I made to describeRoles() (du-attributes_1.diff.bz2), and a combined patch against HEAD containing the full printTable API changes as well as the changes to describeRoles() (du-attributes-print-table_1.diff.bz2). No memory problems detected by valgrind, and all regression tests passed on x86_64 gentoo. I've added this item to the May CommitFest wiki page. Cheers, BJ [1[ http://archives.postgresql.org/message-id/[EMAIL PROTECTED] du-attributes_1.diff.bz2 Description: BZip2 compressed data du-attributes-print-table_1.diff.bz2 Description: BZip2 compressed data -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
On 21/03/2008, Tom Lane [EMAIL PROTECTED] wrote: Brendan Jurd [EMAIL PROTECTED] writes: We can't just build the output table by hand like describeOneTableDetails does? Admittedly it's kludgy, but it's not an unprecedented kludge. Oh, I had forgotten the existence of that entry point in print.c. Yeah, it might be workable --- want to have a go at it? I've had a chance to look at this now, and although it certainly does seem workable, there's a lot of duplication of code that I feel uneasy about. describeOneTableDetails essentially already duplicates the table buildling code in printQuery, so I would be creating a third copy of the same logic. This makes me wonder whether print.c could offer something a bit more helpful to callers wishing to DIY a table; we could have a table-building struct with methods like addHeader and addCell. What do you think? Overkill, or worthy pursuit? - Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
Brendan Jurd [EMAIL PROTECTED] writes: I've had a chance to look at this now, and although it certainly does seem workable, there's a lot of duplication of code that I feel uneasy about. describeOneTableDetails essentially already duplicates the table buildling code in printQuery, so I would be creating a third copy of the same logic. This makes me wonder whether print.c could offer something a bit more helpful to callers wishing to DIY a table; we could have a table-building struct with methods like addHeader and addCell. What do you think? Overkill, or worthy pursuit? Once you have two occurrences of a pattern, it's reasonable to assume there will be more later. +1 for building a little bit of infrastructure. regards, tom lane - Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
Brendan Jurd [EMAIL PROTECTED] writes: I've done up a patch per Tom's idea of combining the binary role attributes into a single column. I started to look at committing this and realized that there's a very nasty problem: our current approach to localizing the strings won't work. See this patch for background: http://archives.postgresql.org/pgsql-committers/2007-12/msg00187.php The code is now set up so that it can pass an entire field value through gettext(), but if gettext recognizes the strings foo and bar that doesn't mean it will do anything useful with foo\nbar, which is what this patch would require. I suspect that to solve this in a non-kluge fashion we'd need to make \du pull over the plain boolean and integer values, then build a new PGresult data structure on its own. Ugh. (Actually, without any support from libpq for building PGresults, it's hard to imagine doing that in a way that wouldn't be a kluge itself.) Or we could go back to the drawing board on what the output ought to look like. Thoughts? regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
Brendan Jurd escribió:
I've done up a patch per Tom's idea of combining the binary role
attributes into a single column.
Thanks -- this is nice. I even went to apply it, but found a problem:
gettext localizes the NULL string to the localization header :-( For
example:
alvherre=# \du
Liste des rôles
Nom du rôle | Attributes |
Membre de
-+-+---
alvherre| Superuser | {}
: Create role
: Create DB
asd | No inherit | {}
: No login
bar | Project-Id-Version: psql| {}
: Report-Msgid-Bugs-To:
: POT-Creation-Date: 2007-12-17 11:14-0400
: PO-Revision-Date: 2007-12-18 10:44+0100
: Last-Translator: Guillaume Lelarge [EMAIL PROTECTED]
: Language-Team: [EMAIL PROTECTED]
: MIME-Version: 1.0
: Content-Type: text/plain; charset=ISO-8859-15
: Content-Transfer-Encoding: 8bit
: X-Generator: KBabel 1.11.4
:
foo | No login|
{asd}
(4 lignes)
Starting psql with LC_ALL=C shows the truth:
alvherre=# \du
List of roles
Role name | Attributes | Member of
---+-+---
alvherre | Superuser | {}
: Create role
: Create DB
asd | No inherit | {}
: No login
bar | | {}
foo | No login| {asd}
(4 rows)
I'm not sure how to fix this. Thoughts?
--
Alvaro Herrerahttp://www.advogato.org/person/alvherre
Management by consensus: I have decided; you concede.
(Leonard Liu)
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
Alvaro Herrera [EMAIL PROTECTED] writes: gettext localizes the NULL string to the localization header :-( For example: Oooh, that's even different from the one I thought of :-(. Yeah, we've got a problem here. We could fix that particular issue by changing print.c so that it doesn't attempt to localize a zero-length string; but that won't help localizing multiple strings that have been concatenated. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
On 21/03/2008, Tom Lane [EMAIL PROTECTED] wrote: The code is now set up so that it can pass an entire field value through gettext(), but if gettext recognizes the strings foo and bar that doesn't mean it will do anything useful with foo\nbar, which is what this patch would require. Ouch! I suspect that to solve this in a non-kluge fashion we'd need to make \du pull over the plain boolean and integer values, then build a new PGresult data structure on its own. Ugh. (Actually, without any support from libpq for building PGresults, it's hard to imagine doing that in a way that wouldn't be a kluge itself.) Or we could go back to the drawing board on what the output ought to look like. We can't just build the output table by hand like describeOneTableDetails does? Admittedly it's kludgy, but it's not an unprecedented kludge. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
Brendan Jurd [EMAIL PROTECTED] writes: On 21/03/2008, Tom Lane [EMAIL PROTECTED] wrote: The code is now set up so that it can pass an entire field value through gettext(), but if gettext recognizes the strings foo and bar that doesn't mean it will do anything useful with foo\nbar, which is what this patch would require. Ouch! We can't just build the output table by hand like describeOneTableDetails does? Admittedly it's kludgy, but it's not an unprecedented kludge. Oh, I had forgotten the existence of that entry point in print.c. Yeah, it might be workable --- want to have a go at it? regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
On 21/03/2008, Tom Lane [EMAIL PROTECTED] wrote: Brendan Jurd [EMAIL PROTECTED] writes: We can't just build the output table by hand like describeOneTableDetails does? Admittedly it's kludgy, but it's not an unprecedented kludge. Oh, I had forgotten the existence of that entry point in print.c. Yeah, it might be workable --- want to have a go at it? Sure. It will be at least a few days (Easter holidays) before I can submit anything. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
Brendan Jurd escribió: I've done up a patch per Tom's idea of combining the binary role attributes into a single column. This patch seems to be missing from the queue. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your Subscription: http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
Alvaro Herrera wrote: Brendan Jurd escribi?: I've done up a patch per Tom's idea of combining the binary role attributes into a single column. This patch seems to be missing from the queue. Yes because I am still processing email from two weeks ago. I was in Europe for a week. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your Subscription: http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---
Brendan Jurd wrote:
I've done up a patch per Tom's idea of combining the binary role
attributes into a single column.
Each attribute which differs from the default is listed on a separate
line, like so:
List of roles
Role name | Attributes | Member of
-++---
bob || {readers,writers}
brendanjurd | Superuser | {}
: Create role
: Create DB
harry | No inherit | {}
jim | 10 connections | {readers}
readers | No login | {}
writers | No login | {}
(6 rows)
Notes:
* The patch relies on array_to_string's current treatment of NULL
values in the array; they are ignored. If that behaviour changes in
the future, the \du output will become very ugly indeed.
* I'm not sure whether No login and No inherit are the best
phrases to use. I took my cue from the SQL setting names NOLOGIN and
NOINHERIT, but maybe something more grammatically sensible with
Cannot login and No inheritance would be preferable.
* If accepted, this patch would supercede the earlier patch mentioned
by Bernd Helmle upthread, which adds LOGIN to the output as a new
column: http://archives.postgresql.org/pgsql-patches/2007-11/msg00014.php
Cheers,
BJ
[ Attachment, skipping... ]
---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?
http://www.postgresql.org/docs/faq
--
Bruce Momjian [EMAIL PROTECTED]http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your Subscription:
http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
I've done up a patch per Tom's idea of combining the binary role
attributes into a single column.
Each attribute which differs from the default is listed on a separate
line, like so:
List of roles
Role name | Attributes | Member of
-++---
bob || {readers,writers}
brendanjurd | Superuser | {}
: Create role
: Create DB
harry | No inherit | {}
jim | 10 connections | {readers}
readers | No login | {}
writers | No login | {}
(6 rows)
Notes:
* The patch relies on array_to_string's current treatment of NULL
values in the array; they are ignored. If that behaviour changes in
the future, the \du output will become very ugly indeed.
* I'm not sure whether No login and No inherit are the best
phrases to use. I took my cue from the SQL setting names NOLOGIN and
NOINHERIT, but maybe something more grammatically sensible with
Cannot login and No inheritance would be preferable.
* If accepted, this patch would supercede the earlier patch mentioned
by Bernd Helmle upthread, which adds LOGIN to the output as a new
column: http://archives.postgresql.org/pgsql-patches/2007-11/msg00014.php
Cheers,
BJ
*** src/bin/psql/describe.c
--- src/bin/psql/describe.c
***
*** 1611,1638 describeRoles(const char *pattern, bool verbose)
PQExpBufferData buf;
PGresult *res;
printQueryOpt myopt = pset.popt;
! static const bool trans_columns[] = {false, true, true, true, true,
false, false};
initPQExpBuffer(buf);
printfPQExpBuffer(buf,
SELECT r.rolname AS \%s\,\n
! CASE WHEN r.rolsuper THEN '%s' ELSE '%s' END
AS \%s\,\n
!CASE WHEN r.rolcreaterole THEN '%s' ELSE '%s' END AS
\%s\,\n
! CASE WHEN r.rolcreatedb THEN '%s' ELSE '%s' END AS
\%s\,\n
! CASE WHEN r.rolconnlimit 0 THEN CAST('%s' AS
pg_catalog.text)\n
!ELSE CAST(r.rolconnlimit AS
pg_catalog.text)\n
! END AS \%s\, \n
ARRAY(SELECT b.rolname FROM
pg_catalog.pg_auth_members m JOIN pg_catalog.pg_roles b ON (m.roleid = b.oid)
WHERE m.member = r.oid) as \%s\,
gettext_noop(Role name),
- gettext_noop(yes),
gettext_noop(no),
gettext_noop(Superuser),
! gettext_noop(yes),
gettext_noop(no),
gettext_noop(Create role),
- gettext_noop(yes),
gettext_noop(no),
gettext_noop(Create DB),
! gettext_noop(no limit),
! gettext_noop(Connections),
gettext_noop(Member of));
if (verbose)
--- 1611,1639
PQExpBufferData buf;
PGresult *res;
printQueryOpt myopt = pset.popt;
! static const bool trans_columns[] = {false, true, false, false};
initPQExpBuffer(buf);
printfPQExpBuffer(buf,
SELECT r.rolname AS \%s\,\n
! array_to_string(ARRAY[\n
! CASE WHEN r.rolsuper THEN '%s' ELSE NULL
END,\n
! CASE WHEN NOT r.rolinherit THEN '%s' ELSE NULL END,\n
! CASE WHEN r.rolcreaterole THEN '%s' ELSE NULL END,\n
!CASE WHEN r.rolcreatedb THEN '%s' ELSE NULL
END,\n
!CASE WHEN NOT r.rolcanlogin THEN '%s' ELSE NULL END,\n
! CASE WHEN r.rolconnlimit = 0 THEN
r.rolconnlimit::pg_catalog.text || ' %s' ELSE NULL END\n
! ], chr(10)) AS \%s\,\n
ARRAY(SELECT b.rolname FROM
pg_catalog.pg_auth_members m JOIN pg_catalog.pg_roles b ON (m.roleid = b.oid)
WHERE m.member = r.oid) as \%s\,
gettext_noop(Role name),
gettext_noop(Superuser),
! gettext_noop(No inherit),
gettext_noop(Create role),
gettext_noop(Create DB),
! gettext_noop(No login),
! gettext_noop(connections),
! gettext_noop(Attributes),
gettext_noop(Member of));
if (verbose)
---(end of
