Re: pg_dump roles support

On Wed, Nov 5, 2008 at 5:00 PM, Dave Page <dpage@pgadmin.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.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
>



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