On Tue, Jan 14, 2014 at 12:22:30PM +0530, Jeevan Chalke wrote:
> On Mon, Jan 13, 2014 at 4:30 PM, Oskari Saarenmaa <os@ohmu.fi> wrote:
> > On 13/01/14 10:26, Jeevan Chalke wrote:
> >
> > > 1. Documentation is missing and thus becomes difficult to understand
> > > what exactly you are trying to do. Or in other words, user will be
> > > uncertain about using it more efficiently.
> >
> > I figured I'd write documentation for this if it looks like a useful
> > feature which would be accepted for 9.4, but I guess it would've
> > helped to have a bit better description of this for the initial
> > submission as well.
> >
> >
> > > 2. Some more comments required. At each new function and
> > > specifically at get_sqlstate_error_level().
> >
> > Just after I submitted the patch I noticed that I had a placeholder
> > for comment about that function but never wrote the actual comment,
> > sorry about that.
> >
> > > 3. Please add test-case if possible.
> >
> > Sure.
> >
> > > 4. Some code part does not comply with PostgreSQL indentation style.
> > > (Can be ignored as it will pass through pg_indent, but better fix
> > > it).
> > >
> >
> > I'll try to fix this for v2.
> >
> > > 5. You have used ""XX000:warning," string to get maximum possible
> > > length of the valid sqlstate:level identifier. It's perfect, but
> > > small explanation about that will be good there. Also in future if
> > > we have any other error level which exceeds this, we need changes
> > > here too. Right ?
> >
> > Good point, I'll address this in v2.
> >
> > > I will look into this further. But please have your attention on above
> > > points.
> >
> > Thanks for the review!
>
> Since you are taking care of most of the points above. I will wait for v2
> patch. Till then marking "Waiting on Author".
Attached v2 of the patch which addresses the above points. I couldn't
figure out how to test log output, but at least the patch now tests that
it can set and show the log level.
Thanks,
Oskari