Content-Type: multipart/alternative;
	boundary="------------5T6Pk0Rd4GGc1Ep4eHEdi8gC"
Message-ID: <03ee95c3-6496-90b7-5c8b-13fe69ee73dd@amazon.com>
Date: Tue, 6 Sep 2022 10:54:35 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.2.1
Subject: Re: more descriptive message for process termination due to
 max_slot_wal_keep_size
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
CC: <sawada.mshk@gmail.com>, <ashutosh.bapat.oss@gmail.com>,
	<pgsql-hackers@postgresql.org>
References: <20220104.102931.1400105396066285349.horikyota.ntt@gmail.com>
 <20220302.153719.171492355527427161.horikyota.ntt@gmail.com>
 <ddee3c71-4e53-94c8-d823-3cee11c63c07@amazon.com>
 <20220906.145336.1494945638376688936.horikyota.ntt@gmail.com>
Content-Language: en-US
From: "Drouvot, Bertrand" <bdrouvot@amazon.com>
In-Reply-To: <20220906.145336.1494945638376688936.horikyota.ntt@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/03ee95c3-6496-90b7-5c8b-13fe69ee73dd%40amazon.com>
Precedence: bulk

--------------5T6Pk0Rd4GGc1Ep4eHEdi8gC
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

Hi,

On 9/6/22 7:53 AM, Kyotaro Horiguchi wrote:
> At Mon, 5 Sep 2022 11:56:33 +0200, "Drouvot, Bertrand"<bdrouvot@amazon.com>  wrote in
>> Hi,
>>
>> On 3/2/22 7:37 AM, Kyotaro Horiguchi wrote:
>>> At Tue, 04 Jan 2022 10:29:31 +0900 (JST), Kyotaro
>>> Horiguchi<horikyota.ntt@gmail.com>  wrote in
>>>> So what do you say if I propose the following?
>>>>
>>>> LOG:  terminating process %d to release replication slot \"%s\"
>>>> because its restart_lsn %X/%X exceeds the limit %X/%X
>>>> HINT: You might need to increase max_slot_wal_keep_size.
>>> This version emits the following message.
>>>
>>> [35785:checkpointer] LOG: terminating process 36368 to release
>>> replication slot "s1" because its restart_lsn 0/1F000148 exceeds the
>>> limit 0/21000000
>>> [35785:checkpointer] HINT: You might need to increase
>>> max_slot_wal_keep_size.
>> As the hint is to increase max_slot_wal_keep_size, what about
>> reporting the difference in size (rather than the limit lsn)?
>> Something along those lines?
>>
>> [35785:checkpointer] LOG: terminating process 36368 to release
>> replication slot "s1" because its restart_lsn 0/1F000148 exceeds the
>> limit by <NNN MB>.
> Thanks! That might be more sensible exactly for the reason you
> mentioned.  One issue doing that is size_pretty is dbsize.c local
> function. Since the size is less than kB in many cases, we cannot use
> fixed unit for that.

Thanks for the new patch version!. I did not realized (sorry about that) 
that we'd need to expose byte_size_pretty(). Now I wonder if we should 
not simply report the number of bytes (like I can see it is done in many 
places). So something like:

@@ -1298,9 +1298,9 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlot *s, 
XLogRecPtr oldestLSN,
                                 byte_size_pretty(buf, sizeof(buf),
oldestLSN - restart_lsn);
                                 ereport(LOG,
- (errmsg("terminating process %d to release replication slot \"%s\" 
because its restart_lsn %X/%X exceeds the limit by %s",
+ (errmsg("terminating process %d to release replication slot \"%s\" 
because its restart_lsn %X/%X exceeds the limit by %lu bytes",
active_pid, NameStr(slotname),
- LSN_FORMAT_ARGS(restart_lsn), buf),
+ LSN_FORMAT_ARGS(restart_lsn), oldestLSN - restart_lsn),
                                                  errhint("You might 
need to increase max_slot_wal_keep_size.")));

and then forget about exposing/using byte_size_pretty() (that would be 
more consistent with the same kind of reporting in the existing code).

What do you think?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services:https://aws.amazon.com

--------------5T6Pk0Rd4GGc1Ep4eHEdi8gC
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: 8bit

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body text="#415ce2" bgcolor="#FFFFFF">
    <p>Hi,<br>
    </p>
    <div class="moz-cite-prefix">On 9/6/22 7:53 AM, Kyotaro Horiguchi
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:20220906.145336.1494945638376688936.horikyota.ntt@gmail.com">
      <pre class="moz-quote-pre" wrap="">At Mon, 5 Sep 2022 11:56:33 +0200, "Drouvot, Bertrand" <a class="moz-txt-link-rfc2396E" href="mailto:bdrouvot@amazon.com">&lt;bdrouvot@amazon.com&gt;</a> wrote in
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">Hi,

On 3/2/22 7:37 AM, Kyotaro Horiguchi wrote:
</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">At Tue, 04 Jan 2022 10:29:31 +0900 (JST), Kyotaro
Horiguchi<a class="moz-txt-link-rfc2396E" href="mailto:horikyota.ntt@gmail.com">&lt;horikyota.ntt@gmail.com&gt;</a> wrote in
</pre>
          <blockquote type="cite">
            <pre class="moz-quote-pre" wrap="">So what do you say if I propose the following?

LOG:  terminating process %d to release replication slot \"%s\"
because its restart_lsn %X/%X exceeds the limit %X/%X
HINT: You might need to increase max_slot_wal_keep_size.
</pre>
          </blockquote>
          <pre class="moz-quote-pre" wrap="">This version emits the following message.

[35785:checkpointer] LOG: terminating process 36368 to release
replication slot "s1" because its restart_lsn 0/1F000148 exceeds the
limit 0/21000000
[35785:checkpointer] HINT: You might need to increase
max_slot_wal_keep_size.
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
As the hint is to increase max_slot_wal_keep_size, what about
reporting the difference in size (rather than the limit lsn)?
Something along those lines?

[35785:checkpointer] LOG: terminating process 36368 to release
replication slot "s1" because its restart_lsn 0/1F000148 exceeds the
limit by &lt;NNN MB&gt;.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Thanks! That might be more sensible exactly for the reason you
mentioned.  One issue doing that is size_pretty is dbsize.c local
function. Since the size is less than kB in many cases, we cannot use
fixed unit for that.</pre>
    </blockquote>
    <p>Thanks for the new patch version!. I did not realized (sorry
      about that) that we'd need to expose byte_size_pretty(). Now I
      wonder if we should not simply report the number of bytes (like I
      can see it is done in many places). So something like:</p>
    <p>@@ -1298,9 +1298,9 @@
      InvalidatePossiblyObsoleteSlot(ReplicationSlot *s, XLogRecPtr
      oldestLSN,<br>
                                      byte_size_pretty(buf, sizeof(buf),<br>
                                                                      
      oldestLSN - restart_lsn);<br>
                                      ereport(LOG,<br>
      -                                              
      (errmsg("terminating process %d to release replication slot \"%s\"
      because its restart_lsn %X/%X exceeds the limit by %s",<br>
      +                                              
      (errmsg("terminating process %d to release replication slot \"%s\"
      because its restart_lsn %X/%X exceeds the limit by %lu bytes",<br>
                                                                     
      active_pid, NameStr(slotname),<br>
      -                                                              
      LSN_FORMAT_ARGS(restart_lsn), buf),<br>
      +                                                              
      LSN_FORMAT_ARGS(restart_lsn), oldestLSN - restart_lsn),<br>
                                                       errhint("You
      might need to increase max_slot_wal_keep_size.")));<br>
      <br>
    </p>
    <p>and then forget about exposing/using byte_size_pretty() (that
      would be more consistent with the same kind of reporting in the
      existing code).<br>
    </p>
    <p>What do you think?<br>
    </p>
    <p>Regards,<br>
    </p>
    <pre class="moz-signature" cols="72">-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: <a class="moz-txt-link-freetext" href="https://aws.amazon.com">https://aws.amazon.com</a></pre>
  </body>
</html>

--------------5T6Pk0Rd4GGc1Ep4eHEdi8gC--