Re: pg_dump roles support

Benedek László <laci@benedekl.tvnetwork.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
notes.

> +        {"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
elsewhere.

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