Skip site navigation (1) Skip section navigation (2)

Re: pg_dump roles support

From: "Ibrar Ahmed" <ibrar(dot)ahmad(at)gmail(dot)com>
To: "Dave Page" <dpage(at)pgadmin(dot)org>, pgsql-rrreviewers(at)postgresql(dot)org
Subject: Re: pg_dump roles support
Date: 2008-11-05 11:09:16
Message-ID: 8494ccf60811050309g6c95be15q21a5433d8663b72b@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-rrreviewers
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

In response to

pgsql-rrreviewers by date

Next:From: Kenneth MarshallDate: 2008-11-05 14:40:56
Subject: Tests citext casts
Previous:From: Dave PageDate: 2008-11-05 11:00:43
Subject: Re: pg_dump roles support

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group