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.ahmad@gmail.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@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-rrreviewers
>



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