Thread: 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(-)
Attachment
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
Hi Benedek. At 2008-11-06 15:08:14 +0100, laci@benedekl.tvnetwork.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
Attachment
[ starting to examine this patch now... ] Benedek László <laci@benedekl.tvnetwork.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@sss.pgh.pa.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