Re: pg_dump roles support [Review]

Just a superficial review. I haven't really looked hard at this yet.

1 - Patch does not apply cleanly on latest git repository, although
there is no hunk failed but there are some hunk succeeded messages.

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.

3 - We should follow the coding standards of existing code

destroyPQExpBuffer(roleQry);
g_fout->rolename = pgrole;
} else {
g_fout->rolename = NULL;
}

Should be written like this

destroyPQExpBuffer(roleQry);
g_fout->rolename = pgrole;
}
else
{
g_fout->rolename = NULL;
}

4 - pg_restore gives error wile restoring custom format backup

pg_restore: [archiver] invalid ROLENAME item: SET role = 'ibrar';
(reason check point 5)

5 - Do you really want to write this code like this

+ if (ptr2)
+ {
+ *ptr2 = '\0';
+ AH->public.rolename = strdup(ptr1);
+ free(defn);5 -
+ }
+ else
+ free(defn);
+ die_horribly(AH, modulename, "invalid ROLENAME item: %s\n",
+ te->defn);

I think you missed curly brackets of else here.

Please send updated patch!

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

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?

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(-)

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

Hi Benedek.

At 2008-11-06 15:08:14 +0100, laci(at)benedekl(dot)tvnetwork(dot)hu wrote:
>
> I created an updated patch according to your notices.

I had a look at your updated patch, and it looks fine. I fiddled with
the documentation a little, and fixed up one place where the code had
drifted and the patch didn't apply.

-- ams

[ starting to examine this patch now... ]

=?UTF-8?B?QmVuZWRlayBMw6FzemzDsw==?= <laci(at)benedekl(dot)tvnetwork(dot)hu> writes:
> 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?

I think that the entire concept of putting the rolename into the archive
is broken, and we should not do that part at all. But we *especially*
should not do it if there is no way to override it.

I see no good reason to assume that the appropriate role to use during
restore is the same as that during dump. We don't reflect the -U
setting into the dump file, and --role is really just an auxiliary
extension to -U. What would make sense is to have a --role switch in
pg_restore, but have that function entirely independently of what
happened at dump time, just as is true for -U.

So my thought is:

--role switch for pg_dump and pg_dumpall: sets the role used while
dumping, has no effect on the emitted archive.

--role switch for pg_restore: sets the role used while restoring,
if it's to be different from what -U says.

This ignores the case of plain-text output from pg_dump, but you
don't really need any support for that case, as you can do the
restore like so:

psql -U admin_user target_db

target_db=> SET ROLE superuser;
target_db=# \i dumpfile.sql

Comments?

regards, tom lane

Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> --role switch for pg_dump and pg_dumpall: sets the role used while
> dumping, has no effect on the emitted archive.
>
> --role switch for pg_restore: sets the role used while restoring,
> if it's to be different from what -U says.

As one of the original requestors for this capability, just wanted to
add my 2c that this will work for me and makes sense to me.

Thanks,

Stephen