Re: control max length of parameter values logged - Mailing list pgsql-hackers

Hi,

Please see the new version attached.
>> +        If greater than zero, bind parameter values reported in non-error
>> +        statement-logging messages are trimmed to no more than this many bytes.
>> +        If this value is specified without units, it is taken as bytes.
>> +        Zero disables logging parameters with statements.
>> +        <literal>-1</literal> (the default) makes parameters logged in full.
> say: "..causes parameters to be logged in full".
>
> Also..I just realized that this truncates *each* parameter, rather than
> truncating the parameter list.
>
> So say: "
> |If greater than zero, each bind parameter value reported in a non-error
> |statement-logging messages is trimmed to no more than this number of bytes.
okay

I also changed "trimmed to no more than this many bytes" to "trimmed to 
this many bytes".
It's not that pedantic any more but hopefully less awkward.

>> +        Non-zero values add some overhead, as
>> +        <productname>PostgreSQL</productname> will compute and store textual
>> +        representations of parameter values in memory for all statements,
>> +        even if they eventually do not get logged.
> say: "even if they are ultimately not logged"
okay

>> +++ b/src/backend/nodes/params.c
>> @@ -280,6 +280,7 @@ BuildParamLogString(ParamListInfo params, char **knownTextValues, int maxlen)
>>                   oldCxt;
>>       StringInfoData buf;
>>   
>> +    Assert(maxlen == -1 || maxlen > 0);
> You can write: >= -1
no, I'm specifically checking they don't pass 0


>> -                    if (log_parameters_on_error)
>> +                    if (log_parameter_max_length_on_error != 0)
> I would write this as >= 0
no, I'm specifically checking for both positives and -1

>> +                        if (log_parameter_max_length_on_error > 0)
>> +                        {
>> +                                                       /*
>> +                                                        * We can trim the saved string, knowing that we
>> +                                                        * won't print all of it.  But we must copy at
>> +                                                        * least two more full characters than
>> +                                                        * BuildParamLogString wants to use; otherwise it
>> +                                                        * might fail to include the trailing ellipsis.
>> +                                                        */
>> +                                                       knownTextValues[paramno] =
>> +                                                               pnstrdup(pstring,
>> +                                                                                log_parameter_max_length_on_error
>> +                                                                                + 2 * MAX_MULTIBYTE_CHAR_LEN);
> The comment says we need at least 2 chars, but
> log_parameter_max_length_on_error might be 1, so I think you can either add 64
> byte fudge factor, like before, or do Max(log_parameter_max_length_on_error, 2).
That's the code I reused without deep analysis TBH.
I think it's mostly for to allocate the space for the ellipsis in case 
it needs to be added,
not to copy any actual characters, that's why we add.

>> +                        }
>> +                        else
>> +                            knownTextValues[paramno] = pstrdup(pstring);
> I suggest to handle the "== -1" case first and the > 0 case as "else".
Good, as long as I thought of this too, I'm changing that.

Best, Alex