Re: pg_dump roles support [Review]

On Thu, Nov 6, 2008 at 8:08 PM, Benedek László
<laci(at)benedekl(dot)tvnetwork(dot)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