Re: pg_dump roles support [Review]

On Thu, Nov 6, 2008 at 8:08 PM, Benedek László
<laci@benedekl.tvnetwork.hu> wrote:
> Hi,
>
> Thanks for your review.
>
> I created an updated patch according to your notices.
>
>> 1 - Patch does not apply cleanly on latest git repository, although
>> there is no hunk failed but there are some hunk succeeded messages.
>
> Rebased to the current HEAD.
>
>> 2- Patch contains unnecessary spaces and tabs which makes the patch
>> unnecessarily big. IMHO please read the patch before sending and make
>> sure that patch only contains the changes you intended to send.
>
> Yes, there were trailing whitespaces in the original files which
> were removed by the previous patch. The attached version leaves them as is.
>
>> 3 - We should follow the coding standards of existing code
>
> I tried, of course, but this escaped my observation.
>
>> 4 - pg_restore gives error wile restoring custom format backup
>> 5 - Do you really want to write this code like this
>
> Fixed.


> I also need some feedback about the role support in pg_restore (not
> implemented yet).
> Currently pg_restore sets the role during the restore process according to
> the TOC
> entry in the archive. It may also support the --role option (just like
> pg_dump).
> If specified it can be used to cancel the effect of the TOC entry and force
> the
> emitting of the SET ROLE ... command. With emtpy argument it can be used to
> omit
> the SET ROLE even if it is specified in the archieve. What do you think?
>

Now this patch looks OK to me. As for as pg_restore is concern I think
we should not add this option into pg_restore.

What advantages do you want to get by using SET ROLE in pg_restore?




> Thank you again.
>
>  doc/src/sgml/ref/pg_dump.sgml        |   16 ++++++++++
>  doc/src/sgml/ref/pg_dumpall.sgml     |   15 +++++++++
>  src/bin/pg_dump/pg_backup.h          |    2 +
>  src/bin/pg_dump/pg_backup_archiver.c |   36 +++++++++++++++++++++-
>  src/bin/pg_dump/pg_dump.c            |   53
> ++++++++++++++++++++++++++++++++++
>  src/bin/pg_dump/pg_dumpall.c         |   23 ++++++++++++++
>  6 files changed, 143 insertions(+), 2 deletions(-)
>
>
>



--   Ibrar Ahmed  EnterpriseDB   http://www.enterprisedb.com