pg_log_backend_memory_contexts() and log level

Started by Fujii Masaoalmost 4 years ago8 messages

masao.fujii@oss.nttdata.com

almost 4 years ago

1 attachment(s)

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

Attachments:

log_level_pg_log_backend_memory_contexts_v1.patchtext/plain; charset=UTF-8; name=log_level_pg_log_backend_memory_contexts_v1.patchDownload

diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 2876f6868c..e12be1b9bd 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -1042,8 +1042,14 @@ ProcessLogMemoryContextInterrupt(void)
 {
 	LogMemoryContextPending = false;
 
-	ereport(LOG,
-			(errmsg("logging memory contexts of PID %d", MyProcPid)));
+	/*
+	 * Use LOG_SERVER_ONLY to prevent this message from being sent to the
+	 * connected client.
+	 */
+	ereport(LOG_SERVER_ONLY,
+			(errhidestmt(true),
+			 errhidecontext(true),
+			 errmsg("logging memory contexts of PID %d", MyProcPid)));
 
 	/*
 	 * When a backend process is consuming huge memory, logging all its memory

Bharath Rupireddy

bharath.rupireddyforpostgres@gmail.com

almost 4 years ago

In reply to: Fujii Masao (#1)

Re: pg_log_backend_memory_contexts() and log level

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]/messages/by-id/CALDaNm3tbc1OKvSKvD5SfmEj66M_sWDPCgDvzFtS9gxRov8jRQ@mail.gmail.com for pg_log_backtrace, let
me quickly send the updated patch there as well. The pg_log_query_plan
on-going patch [2]/messages/by-id/fb6360e4c0ffbdd0a6698b92bb2617e2@oss.nttdata.com, does the right thing.

[1]: /messages/by-id/CALDaNm3tbc1OKvSKvD5SfmEj66M_sWDPCgDvzFtS9gxRov8jRQ@mail.gmail.com
[2]: /messages/by-id/fb6360e4c0ffbdd0a6698b92bb2617e2@oss.nttdata.com

Regards,
Bharath Rupireddy.

Bharath Rupireddy

bharath.rupireddyforpostgres@gmail.com

almost 4 years ago

In reply to: Bharath Rupireddy (#2)

Re: pg_log_backend_memory_contexts() and log level

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] - /messages/by-id/CALDaNm3tbc1OKvSKvD5SfmEj66M_sWDPCgDvzFtS9gxRov8jRQ@mail.gmail.com

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

Regards,
Bharath Rupireddy.

Bharath Rupireddy

bharath.rupireddyforpostgres@gmail.com

almost 4 years ago

In reply to: Bharath Rupireddy (#2)

Re: pg_log_backend_memory_contexts() and log level

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.

torikoshia

torikoshia@oss.nttdata.com

almost 4 years ago

In reply to: Fujii Masao (#1)

Re: pg_log_backend_memory_contexts() and log level

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

Fujii Masao

masao.fujii@oss.nttdata.com

almost 4 years ago

In reply to: Bharath Rupireddy (#4)

Re: pg_log_backend_memory_contexts() and log level

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

Fujii Masao

masao.fujii@oss.nttdata.com

almost 4 years ago

In reply to: torikoshia (#5)

Re: pg_log_backend_memory_contexts() and log level

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

Fujii Masao

masao.fujii@oss.nttdata.com

almost 4 years ago

In reply to: Fujii Masao (#7)

Re: pg_log_backend_memory_contexts() and log level

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