Hello Laurenz,
I liked the idea for this patch. I will also go for the default being
an empty string.
I went through this patch and have some comments on the code,
1. In general, I don't like the idea of goto, maybe we can have a
free_something function to call here.
2.
if (!SplitIdentifierString(new_copy, ',', &states))
{
GUC_check_errdetail("List syntax is invalid.");
goto failed;
}
Here, we don't need all that free-ing, we can just return false here.
3.
/*
* Check the the values are alphanumeric and convert them to upper case
* (SplitIdentifierString converted them to lower case).
*/
for (p = state; *p != '\0'; p++)
if (*p >= 'a' && *p <= 'z')
*p += 'A' - 'a';
else if (*p < '0' || *p > '9')
{
GUC_check_errdetail("error codes can only contain digits and ASCII letters.");
goto failed;
}
I was thinking, maybe we can use tolower() function here.
4.
list_free(states);
pfree(new_copy);
*extra = statelist;
return true;
failed:
list_free(states);
pfree(new_copy);
guc_free(statelist);
return false;
This looks like duplication that can be easily avoided.
You may have free_somthing function to do all free-ing stuff only and
its callee can then have a return statement.
e.g for here,
free_states(states, new_copy, statelist);
return true;
5. Also, for alphanumeric check, maybe we can have something like,
if (isalnum(*state) == 0)
{
GUC_check_errdetail("error codes can only contain digits and ASCII letters.");
goto failed;
}
and we can do this in the beginning after the len check.
On Tue, 18 Jun 2024 at 18:49, Laurenz Albe <laurenz.a...@cybertec.at> wrote:
>
> On Mon, 2024-06-17 at 16:40 -0500, Justin Pryzby wrote:
> > > The feature will become much less useful if unique voilations keep
> > > getting logged.
> >
> > Uh, to be clear, your patch is changing the *defaults*, which I found
> > surprising, even after reaading the thread. Evidently, the current
> > behavior is not what you want, and you want to change it, but I'm *sure*
> > that whatever default you want to use at your site/with your application
> > is going to make someone else unhappy. I surely want unique violations
> > to be logged, for example.
>
> I was afraid that setting the default non-empty would cause objections.
>
> > > + <para>
> > > + This setting is useful to exclude error messages from the log
> > > that are
> > > + frequent but irrelevant.
> >
> > I think you should phrase the feature as ".. *allow* skipping error
> > logging for messages ... that are frequent but irrelevant for a given
> > site/role/DB/etc."
>
> I have reworded that part.
>
> > I suggest that this patch should not change the default behavior at all:
> > its default should be empty. That you, personally, would plan to
> > exclude this or that error code is pretty uninteresting. I think the
> > idea of changing the default behavior will kill the patch, and even if
> > you want to propose to do that, it should be a separate discussion.
> > Maybe you should make it an 002 patch.
>
> I have attached a new version that leaves the parameter empty by default.
>
> The patch is not motivated by my personal dislike of certain error messages.
> A well-written application would not need that parameter at all.
> The motivation for me is based on my dealing with customers' log files,
> which are often full of messages that are only distracting from serious
> problems and fill up the disk.
>
> But you are probably right that it would be hard to find a default setting
> that nobody has quibbles with, and the default can always be changed with
> a future patch.
>
> Yours,
> Laurenz Albe
--
Regards,
Rafia Sabih
