Re: Reducing the log spam

From: Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>
To: Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>
Cc: Justin Pryzby <pryzby(at)telsasoft(dot)com>, Jelte Fennema-Nio <postgres(at)jeltef(dot)nl>, Isaac Morland <isaac(dot)morland(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Reducing the log spam
Date: 2024-07-25 16:03:23
Message-ID: f01c104263606e173c418116a78988fcedee9b61.camel@cybertec.at
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Thanks for the review!

On Wed, 2024-07-24 at 15:27 +0200, Rafia Sabih wrote:
> 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.

The PostgreSQL code base has over 3000 goto's...

Sure, that can be factored out to a function (except the final "return"),
but I feel that a function for three "free" calls is code bloat.

Do you think that avoiding the goto and using a function would make the
code simpler and clearer?

> 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.

I am OK with changing that; I had thought it was more clearer and more
foolproof to use the same pattern everywhere.

> 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.

That is a good idea, but these C library respect the current locale.
I would have to explicitly specify the C locale or switch the locale
temporarily.

Switching the locale seems clumsy, and I have no idea what I would have
to feed as second argument to toupper_l() or isalnum_l().
Do you have an idea?

> 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;

That free_states() function would just contain two function calls.
I think that defining a special function for that is somewhat out of
proportion.

> 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.

isalnum() operates on a single character and depends on the current locale.
See my comments to 3. above.

Please let me know what you think, particularly about the locale problem.

Yours,
Laurenz Albe

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Dunstan 2024-07-25 16:25:49 Re: Sporadic connection-setup-related test failures on Cygwin in v15-
Previous Message Paul Jungwirth 2024-07-25 15:52:44 Re: SQL:2011 application time