| From: | "Euler Taveira" <euler(at)eulerto(dot)com> |
|---|---|
| To: | "Alvaro Herrera" <alvherre(at)alvh(dot)no-ip(dot)org> |
| Cc: | japin <japinli(at)hotmail(dot)com>, "Andres Freund" <andres(at)anarazel(dot)de>, pgsql-hackers(at)lists(dot)postgresql(dot)org, "Chao Li" <li(dot)evan(dot)chao(at)gmail(dot)com> |
| Subject: | Re: log_min_messages per backend type |
| Date: | 2026-02-06 15:45:20 |
| Message-ID: | 88d8e984-ad63-48c1-86af-d50d7776942f@app.fastmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Fri, Feb 6, 2026, at 8:05 AM, Alvaro Herrera wrote:
> Here are some fixups for the main patch. I include yours so that CI
> will test this one.
>
All suggestions seem good to me.
> * I'm not fond of the term "generic", so I changed it to "default",
> which IMO makes more sense from the user point of view.
>
I'm fine with "default" instead of "generic". However, you forgot to replace
"generic" in a few places. The attached patch fixes them all.
> * There seemed to be too much guc_mallocking[*] going on; for instance for
> ptype. I simplified that by just overwriting the : to a '\0', then we
> can use the original string for pg_strcasecmp(). At the bottom of the
> loop we restore the ':', so that the code below can reuse the original
> values. This is okay because we have already mallocked a copy for
> SplitGUCList.
>
Nice trick.
> * There's no reason to add a WARNING to every process type in the
> proctypelist.h table. We can just set the values to WARNING in the
> log_min_messages initializer in guc_tables.c.
>
Ok.
> * There's no reason AFAICS to have log_min_messages_process_types in
> guc_tables.c; nobody else uses it. I made it a local array of char*
> inside the check_log_min_messages function.
>
That's true.
> * There's no need to initialize the newlevel[] array, since we're going
> to assign values to every individual element at one point or another.
> And for the assigned[] array, we don't need to set the value of each to
> 'false' in a loop; we can just initialize to '{0}' and the compiler will
> do the needful. With this we can remove the loop entirely.
>
Ok.
> * "guc_free()+list_free()+return false" the whole time in the various
> failure places across the loop was tiresome. I put them all in a single
> place and jump there with a goto.
>
I tend to avoid goto. However, in this case, it seems it improves readability.
> * in foreach() loops, tracking of a 'bool first' is unnecessary. You
> can just compare foreach_current_index() with 0.
>
Didn't know about foreach_current_index.
> * Rewrote docs and comments
>
Ok.
I attached the same patches that you shared plus the s/generic/default/ that I
said earlier for the sake of CI.
--
Euler Taveira
EDB https://www.enterprisedb.com/
| Attachment | Content-Type | Size |
|---|---|---|
| v11-0001-log_min_messages-per-process-type.patch | text/x-patch | 27.4 KB |
| v11-0002-review.patch | text/x-patch | 17.6 KB |
| v11-0003-fixup-review.patch | text/x-patch | 2.4 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Matheus Alcantara | 2026-02-06 15:46:11 | Re: Add CREATE SCHEMA ... LIKE support |
| Previous Message | Matheus Alcantara | 2026-02-06 15:44:46 | Re: Add CREATE SCHEMA ... LIKE support |