Re: Reducing the log spam - Mailing list pgsql-hackers
From | Rafia Sabih |
---|---|
Subject | Re: Reducing the log spam |
Date | |
Msg-id | CA+FpmFd7gnz3LxoeRWtH1k8JdmitdEw6F6Qg_31HCGijuTNtsQ@mail.gmail.com Whole thread Raw |
In response to | Re: Reducing the log spam (Laurenz Albe <laurenz.albe@cybertec.at>) |
Responses |
Re: Reducing the log spam
|
List | pgsql-hackers |
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.albe@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
pgsql-hackers by date: