Horton

Thread

Commits

  1. Prevent memory context logging from sending log message to connected client.

  1. pg_log_backend_memory_contexts() and log level

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2022-01-26T04:17:43Z

    Hi,

pg_log_backend_memory_contexts() should be designed not to send the messages about the memory contexts to the client regardless of client_min_messages. But I found that the message "logging memory contexts of PID %d" can be sent to the client because it's ereport()'d with LOG level instead of LOG_SERVER_ONLY. Is this a bug, and shouldn't we use LOG_SERVER_ONLY level to log that message? Patch attached.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  2. Re: pg_log_backend_memory_contexts() and log level

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-01-26T05:16:22Z

    On Wed, Jan 26, 2022 at 9:48 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> Hi,
>
> pg_log_backend_memory_contexts() should be designed not to send the messages about the memory contexts to the client regardless of client_min_messages. But I found that the message "logging memory contexts of PID %d" can be sent to the client because it's ereport()'d with LOG level instead of LOG_SERVER_ONLY. Is this a bug, and shouldn't we use LOG_SERVER_ONLY level to log that message? Patch attached.

+1. The patch LGTM.

The same applies for the on-going patch [1] for pg_log_backtrace, let
me quickly send the updated patch there as well. The pg_log_query_plan
on-going patch [2], does the right thing.

[1] - https://www.postgresql.org/message-id/CALDaNm3tbc1OKvSKvD5SfmEj66M_sWDPCgDvzFtS9gxRov8jRQ%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/fb6360e4c0ffbdd0a6698b92bb2617e2%40oss.nttdata.com

Regards,
Bharath Rupireddy.

  3. Re: pg_log_backend_memory_contexts() and log level

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-01-26T05:20:53Z

    On Wed, Jan 26, 2022 at 10:46 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Jan 26, 2022 at 9:48 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >
> > Hi,
> >
> > pg_log_backend_memory_contexts() should be designed not to send the messages about the memory contexts to the client regardless of client_min_messages. But I found that the message "logging memory contexts of PID %d" can be sent to the client because it's ereport()'d with LOG level instead of LOG_SERVER_ONLY. Is this a bug, and shouldn't we use LOG_SERVER_ONLY level to log that message? Patch attached.
>
> +1. The patch LGTM.
>
> The same applies for the on-going patch [1] for pg_log_backtrace, let
> me quickly send the updated patch there as well.
>
> [1] - https://www.postgresql.org/message-id/CALDaNm3tbc1OKvSKvD5SfmEj66M_sWDPCgDvzFtS9gxRov8jRQ%40mail.gmail.com

My bad, the v17 patch of pg_log_backtrace at [1] does emit at LOG_SERVER_ONLY.

Regards,
Bharath Rupireddy.

  4. Re: pg_log_backend_memory_contexts() and log level

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-01-26T05:27:00Z

    On Wed, Jan 26, 2022 at 10:46 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Jan 26, 2022 at 9:48 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >
> > Hi,
> >
> > pg_log_backend_memory_contexts() should be designed not to send the messages about the memory contexts to the client regardless of client_min_messages. But I found that the message "logging memory contexts of PID %d" can be sent to the client because it's ereport()'d with LOG level instead of LOG_SERVER_ONLY. Is this a bug, and shouldn't we use LOG_SERVER_ONLY level to log that message? Patch attached.
>
> +1. The patch LGTM.

While we are here, I think it's enough to have the
has_function_privilege, not needed to set role regress_log_memory and
then execute the function as we already have code covering the
function above (SELECT
pg_log_backend_memory_contexts(pg_backend_pid());). Thought?

SELECT has_function_privilege('regress_log_memory',
  'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- yes

SET ROLE regress_log_memory;
SELECT pg_log_backend_memory_contexts(pg_backend_pid());
RESET ROLE;

Regards,
Bharath Rupireddy.

  5. Re: pg_log_backend_memory_contexts() and log level

    torikoshia <torikoshia@oss.nttdata.com> — 2022-01-26T05:28:43Z

    On 2022-01-26 13:17, Fujii Masao wrote:
> Hi,
> 
> pg_log_backend_memory_contexts() should be designed not to send the
> messages about the memory contexts to the client regardless of
> client_min_messages. But I found that the message "logging memory
> contexts of PID %d" can be sent to the client because it's ereport()'d
> with LOG level instead of LOG_SERVER_ONLY. Is this a bug, and
> shouldn't we use LOG_SERVER_ONLY level to log that message? Patch
> attached.
> 
> Regards,

Thanks! I think it's a bug.

There are two clients: "the client that executed 
pg_log_backend_memory_contexts()" and "the client that issued the query 
that was the target of the memory context logging", but I was only 
concerned about the former when I wrote that part of the code.
The latter is not expected.

It seems better to use LOG_SERVER_ONLY as the attached patch.

The patch was successfully applied and there was no regression test 
error on my environment.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

  6. Re: pg_log_backend_memory_contexts() and log level

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2022-01-27T03:45:10Z

    
On 2022/01/26 14:27, Bharath Rupireddy wrote:
> On Wed, Jan 26, 2022 at 10:46 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> On Wed, Jan 26, 2022 at 9:48 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>
>>> Hi,
>>>
>>> pg_log_backend_memory_contexts() should be designed not to send the messages about the memory contexts to the client regardless of client_min_messages. But I found that the message "logging memory contexts of PID %d" can be sent to the client because it's ereport()'d with LOG level instead of LOG_SERVER_ONLY. Is this a bug, and shouldn't we use LOG_SERVER_ONLY level to log that message? Patch attached.
>>
>> +1. The patch LGTM.

Thanks for the review!


> While we are here, I think it's enough to have the
> has_function_privilege, not needed to set role regress_log_memory and
> then execute the function as we already have code covering the
> function above (SELECT
> pg_log_backend_memory_contexts(pg_backend_pid());). Thought?
> 
> SELECT has_function_privilege('regress_log_memory',
>    'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- yes
> 
> SET ROLE regress_log_memory;
> SELECT pg_log_backend_memory_contexts(pg_backend_pid());
> RESET ROLE;

Or it's enough to set role and execute the function? Either those or has_function_privilege() can be removed from the test, but I don't have strong reason to do that for now...

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  7. Re: pg_log_backend_memory_contexts() and log level

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2022-01-27T03:45:33Z

    
On 2022/01/26 14:28, torikoshia wrote:
> On 2022-01-26 13:17, Fujii Masao wrote:
>> Hi,
>>
>> pg_log_backend_memory_contexts() should be designed not to send the
>> messages about the memory contexts to the client regardless of
>> client_min_messages. But I found that the message "logging memory
>> contexts of PID %d" can be sent to the client because it's ereport()'d
>> with LOG level instead of LOG_SERVER_ONLY. Is this a bug, and
>> shouldn't we use LOG_SERVER_ONLY level to log that message? Patch
>> attached.
>>
>> Regards,
> 
> Thanks! I think it's a bug.
> 
> There are two clients: "the client that executed pg_log_backend_memory_contexts()" and "the client that issued the query that was the target of the memory context logging", but I was only concerned about the former when I wrote that part of the code.
> The latter is not expected.
> 
> It seems better to use LOG_SERVER_ONLY as the attached patch.
> 
> The patch was successfully applied and there was no regression test error on my environment.

Thanks for the review! So barring any objection, I will commit the patch and backport it to v14 where pg_log_backend_memory_contexts() is added.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  8. Re: pg_log_backend_memory_contexts() and log level

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2022-01-28T02:27:39Z

    
On 2022/01/27 12:45, Fujii Masao wrote:
> Thanks for the review! So barring any objection, I will commit the patch and backport it to v14 where pg_log_backend_memory_contexts() is added.

Pushed. Thanks!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION