Re: pg_dump roles support

Please send reviews to pgsql-hackers, *not* this list.

On Wed, Nov 5, 2008 at 4:21 AM, Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com> wrote:
> 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
>
> --
> Sent via pgsql-rrreviewers mailing list (pgsql-rrreviewers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-rrreviewers
>

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com