Subject: Re: Truncate logs by max_log_size

From: Kirill Gavrilov <[email-masked]>
Date: Fri, 29 Nov 2024 23:57:38 +0300

On Fri, Nov 29, 2024 at 10:12 PM Jim Jones <[email-masked]>
wrote:

>
>
> On 29.11.24 12:05, Kirill Gavrilov wrote:
> >   Setting max_log_size to 0 already disables truncation.
> >   I changed guc units to bytes, just to make it more human-friendly.
> > Did some refactoring and fixed all warnings. Set default value in
> > postgresql.conf.
>
> Nice. The patch applies cleanly and the warnings are gone.
>
> A few other things:
>
> IMHO the documentation can be a bit clearer. It says to input the values
> in bytes, but the default value is in megabytes.
>
> +        If greater than zero, each query logged is truncated to this
> many bytes.
> +        Zero disables the setting.
> +        Default value is 5MB.
>
> Perhaps something like "If this value is specified without units, it is
> taken as bytes." (see wal_keep_size)
>

Added to documentation. I hope it's clear now.

>
> And a pedantic note: This hint suggests that you can use TB as unit ..
>
> postgres=# set max_log_size = '1foo';
> ERROR:  invalid value for parameter "max_log_size": "1foo"
> HINT:  Valid units for this parameter are "B", "kB", "MB", "GB", and "TB".
>
> .. but it can never be used, as it exceeds the integer range:
>
> postgres=# SET max_log_size = '1TB';
> ERROR:  invalid value for parameter "max_log_size": "1TB"
> HINT:  Value exceeds integer range.
>
> Thanks
>
> --
> Jim
>

Same thing applies to log_parameter_max_length, for example.

postgres=# set log_parameter_max_length = '1foo';
ERROR:  invalid value for parameter "log_parameter_max_length": "1foo"
HINT:  Valid units for this parameter are "B", "kB", "MB", "GB", and "TB".
postgres=# set log_parameter_max_length = '1TB';
ERROR:  invalid value for parameter "log_parameter_max_length": "1TB"
HINT:  Value exceeds integer range.

I think we can leave it as is.

---

From: Kirill Reshke <[email-masked]>
Date: Sat, 30 Nov 2024 11:58:38 +0500

On Sat, 30 Nov 2024 at 01:57, Kirill Gavrilov <[email-masked]> wrote:
>
> On Fri, Nov 29, 2024 at 10:12 PM Jim Jones <[email-masked]> wrote:
>>
>>
>>

Hi

> +for (my $attempts = 0; $attempts < $max_attempts; $attempts++)
> +{
> + eval {
> + $current_logfiles = slurp_file($node->data_dir . '/current_logfiles');
> + };
> + last unless $@;
> + usleep(100_000);
> +}

`usleep` in tap tests is usually a bad pattern. Do we have a chance to
test this using `wait_for_log` or similar?

-- 
Best regards,
Kirill Reshke

---

From: Jim Jones <[email-masked]>
Date: Mon, 2 Dec 2024 11:08:55 +0100

> I think we can leave it as is.

I see. So I guess it is out of scope to change this message here.

Small nitpicks:

1) The indentation of the comment at postgresql.conf.sample is a little
bit off

#max_log_size = 0                 # max size of logged statement
                    # 0 disables the feature

IMHO it looks better like this:

#max_log_size = 0   # max size of logged statement
                    # 0 disables the feature

2) You introduced a trailing whitespace at L34 (Not critical :))

It happens to me all the time, so I usually try to apply my patches in a
clean branch just to make sure I didn't miss anything.

Other than that, I have nothing more to add at this point.

Thanks

-- 
Jim

---

From: Kirill Gavrilov <[email-masked]>
Date: Fri, 31 Jan 2025 13:46:37 +0300

> Other than that, I have nothing more to add at this point.
Sorry for the long silence.  I fixed the indentation and a trailing
whitespace. Should look fine now.

---

From: Jim Jones <[email-masked]>
Date: Mon, 3 Feb 2025 11:31:07 +0100

Hi Kirill

> whitespace. Should look fine now.

The patch applies cleanly, the documentation is clear, and all tests pass.

It is possible to change this new parameter session-wise, which is nice!

postgres=# SET max_log_size TO 7;
SET
postgres=# SHOW max_log_size;
 max_log_size
--------------
 7B
(1 row)

The default value now is clear and it corresponds to the value set on
postgresql.conf:

postgres=# SHOW max_log_size;
 max_log_size
--------------
 0
(1 row)

Logs are truncated as expected:

postgres=# SET max_log_size TO 6;
SET
postgres=# SELECT length('CALL xyz;');
 length
--------
      9
(1 row)

postgres=# CALL xyz;
ERROR:  syntax error at or near ";"
LINE 1: CALL xyz;

log entry:

2025-02-03 10:58:19.975 CET [123945] ERROR:  syntax error at or near ";"
at character 9
2025-02-03 10:58:19.975 CET [123945] STATEMENT:  CALL x

The issue with log entry sizes for queries containing special characters
was resolved by setting the unit to bytes.

Overall, everythingLGTM.

The new status of this patch is: Ready for Committer

Jim