> On Aug 6, 2026, at 03:44, Peter Eisentraut <[email protected]> wrote:
>
> On 05.08.26 05:36, Chao Li wrote:
>>> On Aug 4, 2026, at 17:36, Peter Eisentraut <[email protected]> wrote:
>>>
>>> These functions are used in psql to assemble tables to print.
>>>
>>> They would sometimes overwrite the string argument they are passed, namely
>>> via mbvalidate(), which removes invalid UTF-8 characters (or potentially
>>> analogously in other encodings, but that is not implemented). However,
>>> many callers are not expecting that. In many callers, the input value
>>> comes directly from libpq structures, such as from PQgetvalue() or
>>> PQsslAttribute(). The latter actually has a const char * return type, and
>>> that was cast away. But even the former is not expecting its return value
>>> to be modified.
>>>
>>> Fix that by making these arguments const. Internally, we add a separate
>>> function that does only the checking part of mbvalidate(). Only if the
>>> validation returns a negative result, we make a copy and run mbvalidate()
>>> on the copy. printTableAddCell() already had internal infrastructure for
>>> keeping track of what values needed to be freed. We add the same for
>>> printTableAddHeader().
>>>
>>> In passing, also simplify the code a bit. There were essentially duplicate
>>> mechanisms for keeping track of the most recently added
>>> cell (fields .cell and .cellsadded). Make that consistent by using an
>>> integer counter for everything. That makes the code arguably easier to
>>> read than with the "current pointer" approaches.
>>>
>>> The first three patches are preparation patches to further clean up the
>>> nearby code a bit.
>>> <0001-Remove-useless-confusing-const-qualifiers.patch><0002-Remove-useless-ENABLE_NLS-conditionals.patch><0003-Use-frontend-logging-API-in-fe_utils-print.c.patch><0004-Make-printTableAddCell-printTableAddHeader-string-ar.patch>
>> 0001, 0002 and 0003 look good to me.
>> For 0001 and 0003, I searched over the source tree, and found a few more
>> occurrences, see the attached diff files.
>
> Ok, I added these. (I added your 0001 to my 0001 and added your 0003 as a
> separate patch.)
>
> Note that your 0001 was incomplete: It did not update the "const bool
> newline" in the add_tablespace_footer() definition.
>
> Also, in your 0003, the messages still contained newlines, but the logging
> API adds its own newlines and in fact rejects strings that end with newlines,
> so that patch wouldn't have worked. I have fixed that.
Sorry about that. I was just about to point out what was missing, so I made
those quick-and-dirty changes without reviewing them as carefully as I would
when proposing a patch.
>
>> For 0004, it seems to introduce a memory leak in printTableAddCell():
>
> Thanks, I have fixed that in the attached patch.
> <v2-0001-Remove-useless-confusing-const-qualifiers.patch><v2-0002-Remove-useless-ENABLE_NLS-conditionals.patch><v2-0003-Use-frontend-logging-API-in-fe_utils-print.c.patch><v2-0004-Make-printTableAddCell-printTableAddHeader-string.patch><v2-0005-Use-frontend-logging-API-in-fe_utils-string_utils.patch>
V2 LGTM.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
