Postgres Pro
Downloads
Home   >   mailing lists

Thread: pg_dump roles support

pg_dump roles support

From
"Ibrar Ahmed"
Date:
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

Re: pg_dump roles support

From
"Dave Page"
Date:
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

Re: pg_dump roles support

From
"Ibrar Ahmed"
Date:
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