Re: pg_dump roles support

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Benedek László <laci(at)benedekl(dot)tvnetwork(dot)hu>
Cc: pgsql-hackers(at)postgresql(dot)org, Stephen Frost <sfrost(at)snowman(dot)net>
Subject: Re: pg_dump roles support
Date: 2008-09-03 19:33:02
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

=?ISO-8859-1?Q?Benedek_L=E1szl=F3?= <laci(at)benedekl(dot)tvnetwork(dot)hu> writes:
> pg_dumpall now just passes the --role option to pg_dump. What do you
> think, is it enough
> or it should issue the SET ROLE TO ... command in its own session too?

I think it would have to, in the general case. Consider the possibility
that someone has restricted access to the system catalogs, for instance.

You have missed an important component of Stephen's original proposal,
which was the point that something similar is needed on the restore
side. This is a little bit tricky since the context at restore time
is not necessarily the same as the context at dump time. When using
an archive file it's not a problem: the behavior can be driven off a
--role switch to pg_restore, and this is independent of what pg_dump
did. In a dump to plain text, though, I'm not sure what to do. The
simplest design would have pg_dump's --role switch control both
what it does in its own connection to the source database, and what it
puts into the output script. I'm not sure that's adequate though.
Is it worth having two different switches for the two cases? If we
think it's a corner case to need different role IDs, we could just
leave it like that and tell anyone who needs different behaviors that
they have to go through an archive file and pg_restore. Stephen,
you were the one who wanted this in the first place, what's your
use-cases look like?

Some other review nitpicking:

The documentation part of the patch is well short of acceptable IMHO,
since it gives no hint of what this switch might be good for, and
indeed encourages the user to confuse it with the -U switch by injecting
a mention of it into the middle of a discussion about -U.

It is not normally considered appropriate for individual patches to
edit the release notes; and it's DEFINITELY not appropriate to put
a mention of a feature addition into the wrong section of the release

> + {"role", required_argument, NULL, 'r' + 0x80},

This is not a good choice of option code IMHO ... what if the value is
stored in a signed char on some machines? If you can't find a free
letter you like, use a small integer code, as you can find being done

BTW, the patch fails to compile on a strict ANSI C compiler, because
you are using a C++-ism of declaring a variable mid-block.

regards, tom lane

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Greg Smith 2008-09-03 20:04:12 Re: [patch] GUC source file and line number]
Previous Message Jan-Peter Seifert 2008-09-03 19:28:05 Re: SSL problems