Thread: Re: [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
Attachment
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 Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
"Brendan Jurd" <direvus@gmail.com> 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
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 <guillaume@lelarge.info>
: Language-Team: <fr@li.org>
: 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 Herrera http://www.advogato.org/person/alvherre
Management by consensus: I have decided; you concede.
(Leonard Liu)
Alvaro Herrera <alvherre@alvh.no-ip.org> 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
On 21/03/2008, Tom Lane <tgl@sss.pgh.pa.us> 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.
"Brendan Jurd" <direvus@gmail.com> writes:
> On 21/03/2008, Tom Lane <tgl@sss.pgh.pa.us> 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
On 21/03/2008, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Brendan Jurd" <direvus@gmail.com> 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.
On 21/03/2008, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Brendan Jurd" <direvus@gmail.com> 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?
"Brendan Jurd" <direvus@gmail.com> 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
On Tue, Mar 25, 2008 at 2:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Brendan Jurd" <direvus@gmail.com> 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/37ed240d0804120112w60c80089obd3a8c8ac72505f8@mail.gmail.com
Attachment
-----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/37ed240d0804150111l14f623bx6fd594dd516bf4a4@mail.gmail.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.7 (GNU/Linux) Comment: http://getfiregpg.org iD8DBQFIBGZP5YBsbHkuyV0RAsb9AKDrFJ/8V00t2XCwIihzEZYcPQZKiQCg3q6L RkiMfjqLay/JLk8phnniYLs= =oW6S -----END PGP SIGNATURE-----