Re: pg_dump roles support

On Wed, Nov 5, 2008 at 5:00 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> Please send reviews to pgsql-hackers, *not* this list.

I have done that, sorry for that.

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

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