From: | "Ibrar Ahmed" <ibrar(dot)ahmad(at)gmail(dot)com> |
---|---|
To: | pgsql-rrreviewers(at)postgresql(dot)org |
Subject: | pg_dump roles support |
Date: | 2008-11-05 09:21:36 |
Message-ID: | 8494ccf60811050121s14e07644x3a4112c0b14c2269@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-rrreviewers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Dave Page | 2008-11-05 11:00:43 | Re: pg_dump roles support |
Previous Message | Brendan Jurd | 2008-11-04 02:24:19 | Re: new assignment for Brendan |