RFC: Logging plan of the running query

st 12. 5. 2021 v 13:24 odesílatel torikoshia <torikoshia@oss.nttdata.com>
napsal:

Hi,

During the discussion about memory contexts dumping[1], there
was a comment that exposing not only memory contexts but also
query plans and untruncated query string would be useful.

I also feel that it would be nice when thinking about situations
such as troubleshooting a long-running query on production
environments where we cannot use debuggers.

At that point of the above comment, I was considering exposing
such information on the shared memory.
However, since memory contexts are now exposed on the log by
pg_log_backend_memory_contexts(PID), I'm thinking about
defining a function that logs the plan of a running query and
untruncated query string on the specified PID in the same way
as below.

postgres=# SELECT * FROM pg_log_current_plan(2155192);
pg_log_current_plan
---------------------
t
(1 row)

$ tail -f data/log/postgresql-2021-05-12.log

2021-05-12 17:37:19.481 JST [2155192] LOG: logging the plan of
running query on PID 2155192
Query Text: SELECT a.filler FROM pgbench_accounts a JOIN
pgbench_accounts b ON a.aid = b.aid;
Merge Join (cost=0.85..83357.85 rows=1000000 width=85)
Merge Cond: (a.aid = b.aid)
-> Index Scan using pgbench_accounts_pkey on
pgbench_accounts a (cost=0.42..42377.43 rows=1000000 width=89)
-> Index Only Scan using pgbench_accounts_pkey on
pgbench_accounts b (cost=0.42..25980.42 rows=1000000 width=4)

Attached a PoC patch.

Any thoughts?

+1

Pavel

On Wed, May 12, 2021 at 4:54 PM torikoshia <torikoshia@oss.nttdata.com> wrote:

Hi,

During the discussion about memory contexts dumping[1], there
was a comment that exposing not only memory contexts but also
query plans and untruncated query string would be useful.

I also feel that it would be nice when thinking about situations
such as troubleshooting a long-running query on production
environments where we cannot use debuggers.

At that point of the above comment, I was considering exposing
such information on the shared memory.
However, since memory contexts are now exposed on the log by
pg_log_backend_memory_contexts(PID), I'm thinking about
defining a function that logs the plan of a running query and
untruncated query string on the specified PID in the same way
as below.

postgres=# SELECT * FROM pg_log_current_plan(2155192);
pg_log_current_plan
---------------------
t
(1 row)

$ tail -f data/log/postgresql-2021-05-12.log

2021-05-12 17:37:19.481 JST [2155192] LOG: logging the plan of
running query on PID 2155192
Query Text: SELECT a.filler FROM pgbench_accounts a JOIN
pgbench_accounts b ON a.aid = b.aid;
Merge Join (cost=0.85..83357.85 rows=1000000 width=85)
Merge Cond: (a.aid = b.aid)
-> Index Scan using pgbench_accounts_pkey on
pgbench_accounts a (cost=0.42..42377.43 rows=1000000 width=89)
-> Index Only Scan using pgbench_accounts_pkey on
pgbench_accounts b (cost=0.42..25980.42 rows=1000000 width=4)

Attached a PoC patch.

Any thoughts?

[1]
/messages/by-id/CA+TgmobkpFV0UB67kzXuD36--OFHwz1bs=L_6PZbD4nxKqUQMw@mail.gmail.com

+1 for the idea. It looks like pg_log_current_plan is allowed to run
by superusers. Since it also shows up the full query text and the plan
in the server log as plain text, there are chances that the sensitive
information might be logged into the server log which is a risky thing
from security standpoint. There's another thread (see [1]/messages/by-id/CA+TgmoZz=K1bQRp0Ug=6uMGFWg-6kaxdHe6VSWaxq0U-YkppYQ@mail.gmail.com below) which
discusses this issue by having a separate role for all debugging
purposes. Note that final consensus is not reached yet. We may want to
use the same role for this patch as well.

[1]: /messages/by-id/CA+TgmoZz=K1bQRp0Ug=6uMGFWg-6kaxdHe6VSWaxq0U-YkppYQ@mail.gmail.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, 12 May 2021 at 13:24, torikoshia <torikoshia@oss.nttdata.com> wrote:

Hi,

During the discussion about memory contexts dumping[1], there
was a comment that exposing not only memory contexts but also
query plans and untruncated query string would be useful.

I also feel that it would be nice when thinking about situations
such as troubleshooting a long-running query on production
environments where we cannot use debuggers.

At that point of the above comment, I was considering exposing
such information on the shared memory.
However, since memory contexts are now exposed on the log by
pg_log_backend_memory_contexts(PID), I'm thinking about
defining a function that logs the plan of a running query and
untruncated query string on the specified PID in the same way
as below.

postgres=# SELECT * FROM pg_log_current_plan(2155192);
pg_log_current_plan
---------------------
t
(1 row)

$ tail -f data/log/postgresql-2021-05-12.log

2021-05-12 17:37:19.481 JST [2155192] LOG: logging the plan of
running query on PID 2155192
Query Text: SELECT a.filler FROM pgbench_accounts a JOIN
pgbench_accounts b ON a.aid = b.aid;
Merge Join (cost=0.85..83357.85 rows=1000000 width=85)
Merge Cond: (a.aid = b.aid)
-> Index Scan using pgbench_accounts_pkey on
pgbench_accounts a (cost=0.42..42377.43 rows=1000000 width=89)
-> Index Only Scan using pgbench_accounts_pkey on
pgbench_accounts b (cost=0.42..25980.42 rows=1000000 width=4)

Attached a PoC patch.

Any thoughts?

Great idea. One feature I'd suggest would be adding a 'format' option
as well, if such feature would be feasable.

With regards,

Matthias van de Meent

On Wed, May 12, 2021 at 08:24:04PM +0900, torikoshia wrote:

Hi,

During the discussion about memory contexts dumping[1], there
was a comment that exposing not only memory contexts but also
query plans and untruncated query string would be useful.

I also feel that it would be nice when thinking about situations
such as troubleshooting a long-running query on production
environments where we cannot use debuggers.

At that point of the above comment, I was considering exposing
such information on the shared memory.
However, since memory contexts are now exposed on the log by
pg_log_backend_memory_contexts(PID), I'm thinking about
defining a function that logs the plan of a running query and
untruncated query string on the specified PID in the same way
as below.

postgres=# SELECT * FROM pg_log_current_plan(2155192);
pg_log_current_plan
---------------------
t
(1 row)

$ tail -f data/log/postgresql-2021-05-12.log

2021-05-12 17:37:19.481 JST [2155192] LOG: logging the plan of running
query on PID 2155192
Query Text: SELECT a.filler FROM pgbench_accounts a JOIN
pgbench_accounts b ON a.aid = b.aid;
Merge Join (cost=0.85..83357.85 rows=1000000 width=85)
Merge Cond: (a.aid = b.aid)
-> Index Scan using pgbench_accounts_pkey on pgbench_accounts a
(cost=0.42..42377.43 rows=1000000 width=89)
-> Index Only Scan using pgbench_accounts_pkey on
pgbench_accounts b (cost=0.42..25980.42 rows=1000000 width=4)

I didn't read the POC patch yet, but +1 for having that feature.

On Wed, 2021-05-12 at 18:03 +0530, Bharath Rupireddy wrote:

On Wed, May 12, 2021 at 4:54 PM torikoshia <torikoshia@oss.nttdata.com> wrote:

During the discussion about memory contexts dumping[1], there
was a comment that exposing not only memory contexts but also
query plans and untruncated query string would be useful.

postgres=# SELECT * FROM pg_log_current_plan(2155192);
pg_log_current_plan
---------------------
t
(1 row)

$ tail -f data/log/postgresql-2021-05-12.log

2021-05-12 17:37:19.481 JST [2155192] LOG: logging the plan of
running query on PID 2155192
Query Text: SELECT a.filler FROM pgbench_accounts a JOIN
pgbench_accounts b ON a.aid = b.aid;
Merge Join (cost=0.85..83357.85 rows=1000000 width=85)
Merge Cond: (a.aid = b.aid)
-> Index Scan using pgbench_accounts_pkey on
pgbench_accounts a (cost=0.42..42377.43 rows=1000000 width=89)
-> Index Only Scan using pgbench_accounts_pkey on
pgbench_accounts b (cost=0.42..25980.42 rows=1000000 width=4)

I love the idea, but I didn't look at the patch.

Since it also shows up the full query text and the plan
in the server log as plain text, there are chances that the sensitive
information might be logged into the server log which is a risky thing
from security standpoint.

I think that is irrelevant.

A superuser can already set "log_statement = 'all'" to get this.
There is no protection from superusers, and it is pointless to require that.

Yours,
Laurenz Albe

Thank you all for your positive comments.

On 2021-05-12 21:55, Matthias van de Meent wrote:

Great idea. One feature I'd suggest would be adding a 'format' option
as well, if such feature would be feasable.

Thanks for the comment!

During the development of pg_log_backend_memory_contexts(), I tried to
make the number of contexts to record configurable by making it GUC
variable or putting it on the shared memory, but the former seemed an
overkill and the latter introduced some ugly behaviors, so we decided
to make it a static number[1]/messages/by-id/6738f309-a41b-cbe6-bb57-a1c58ce9f05a@oss.nttdata.com.
I think we face the same difficulty here.

Allowing to select the format would be better as auto_explain does by
auto_explain.log_format, but I'm a bit doubtful that it is worth the
costs.

[1]: /messages/by-id/6738f309-a41b-cbe6-bb57-a1c58ce9f05a@oss.nttdata.com
/messages/by-id/6738f309-a41b-cbe6-bb57-a1c58ce9f05a@oss.nttdata.com

Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

On 2021-05-13 01:08, Laurenz Albe wrote:

On Wed, 2021-05-12 at 18:03 +0530, Bharath Rupireddy wrote:

Since it also shows up the full query text and the plan
in the server log as plain text, there are chances that the sensitive
information might be logged into the server log which is a risky thing
from security standpoint.

Thanks for the notification!

I think that is irrelevant.

A superuser can already set "log_statement = 'all'" to get this.
There is no protection from superusers, and it is pointless to require
that.

AFAIU, since that discussion is whether or not users other than
superusers
should be given the privilege to execute the backtrace printing
function,
I think it might be applicable to pg_log_current_plan().

Since restricting privilege to superusers is stricter, I'm going to
proceed
as it is for now, but depending on the above discussion, it may be
better to
change it.

Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

On Wed, May 12, 2021 at 4:54 PM torikoshia <torikoshia@oss.nttdata.com> wrote:

Hi,

During the discussion about memory contexts dumping[1], there
was a comment that exposing not only memory contexts but also
query plans and untruncated query string would be useful.

I also feel that it would be nice when thinking about situations
such as troubleshooting a long-running query on production
environments where we cannot use debuggers.

At that point of the above comment, I was considering exposing
such information on the shared memory.
However, since memory contexts are now exposed on the log by
pg_log_backend_memory_contexts(PID), I'm thinking about
defining a function that logs the plan of a running query and
untruncated query string on the specified PID in the same way
as below.

postgres=# SELECT * FROM pg_log_current_plan(2155192);
pg_log_current_plan
---------------------
t
(1 row)

+1 for the idea. I did not read the complete patch but while reading
through the patch, I noticed that you using elevel as LOG for printing
the stack trace. But I think the backend whose pid you have passed,
the connected client to that backend might not have superuser
privileges and if you use elevel LOG then that message will be sent to
that connected client as well and I don't think that is secure. So
can we use LOG_SERVER_ONLY so that we can prevent
it from sending to the client.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 13, 2021 at 2:44 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

+1 for the idea. I did not read the complete patch but while reading
through the patch, I noticed that you using elevel as LOG for printing
the stack trace. But I think the backend whose pid you have passed,
the connected client to that backend might not have superuser
privileges and if you use elevel LOG then that message will be sent to
that connected client as well and I don't think that is secure. So
can we use LOG_SERVER_ONLY so that we can prevent
it from sending to the client.

True, we should use LOG_SERVER_ONLY and not send any logs to the client.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 13, 2021 at 2:57 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Thu, May 13, 2021 at 2:44 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

+1 for the idea. I did not read the complete patch but while reading
through the patch, I noticed that you using elevel as LOG for printing
the stack trace. But I think the backend whose pid you have passed,
the connected client to that backend might not have superuser
privileges and if you use elevel LOG then that message will be sent to
that connected client as well and I don't think that is secure. So
can we use LOG_SERVER_ONLY so that we can prevent
it from sending to the client.

True, we should use LOG_SERVER_ONLY and not send any logs to the client.

I further tend to think that, is it correct to log queries with LOG
level when log_statement GUC is set? Or should it also be
LOG_SERVER_ONLY?

/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG,
(errmsg("statement: %s", query_string),
errhidestmt(true),
errdetail_execute(parsetree_list)));

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 13, 2021 at 1:56 PM torikoshia <torikoshia@oss.nttdata.com> wrote:

On 2021-05-13 01:08, Laurenz Albe wrote:

On Wed, 2021-05-12 at 18:03 +0530, Bharath Rupireddy wrote:

Since it also shows up the full query text and the plan
in the server log as plain text, there are chances that the sensitive
information might be logged into the server log which is a risky thing
from security standpoint.

Thanks for the notification!

I think that is irrelevant.

A superuser can already set "log_statement = 'all'" to get this.
There is no protection from superusers, and it is pointless to require
that.

AFAIU, since that discussion is whether or not users other than
superusers
should be given the privilege to execute the backtrace printing
function,
I think it might be applicable to pg_log_current_plan().

Since restricting privilege to superusers is stricter, I'm going to
proceed
as it is for now, but depending on the above discussion, it may be
better to
change it.

Yeah, we can keep it as superuser-only for now.

Might be unrelated, but just for info - there's another thread
"Granting control of SUSET gucs to non-superusers" at [1]/messages/by-id/F9408A5A-B20B-42D2-9E7F-49CD3D1547BC@enterprisedb.com discussing
the new roles.

[1]: /messages/by-id/F9408A5A-B20B-42D2-9E7F-49CD3D1547BC@enterprisedb.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 13, 2021 at 3:06 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Thu, May 13, 2021 at 2:57 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Thu, May 13, 2021 at 2:44 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

+1 for the idea. I did not read the complete patch but while reading
through the patch, I noticed that you using elevel as LOG for printing
the stack trace. But I think the backend whose pid you have passed,
the connected client to that backend might not have superuser
privileges and if you use elevel LOG then that message will be sent to
that connected client as well and I don't think that is secure. So
can we use LOG_SERVER_ONLY so that we can prevent
it from sending to the client.

True, we should use LOG_SERVER_ONLY and not send any logs to the client.

I further tend to think that, is it correct to log queries with LOG
level when log_statement GUC is set? Or should it also be
LOG_SERVER_ONLY?

/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG,
(errmsg("statement: %s", query_string),
errhidestmt(true),
errdetail_execute(parsetree_list)));

What is your argument behind logging it with LOG? I mean we are
sending the signal to all the backend and some backend might have the
client who is not connected as a superuser so sending the plan to
those clients is not a good idea from a security perspective.
Anyways, LOG_SERVER_ONLY is not an exposed logging level it is used
for an internal purpose. So IMHO it should be logged with
LOG_SERVER_ONLY level.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On 2021-05-13 18:36, Bharath Rupireddy wrote:

On Thu, May 13, 2021 at 2:57 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Thu, May 13, 2021 at 2:44 PM Dilip Kumar <dilipbalaut@gmail.com>
wrote:

+1 for the idea. I did not read the complete patch but while reading
through the patch, I noticed that you using elevel as LOG for printing
the stack trace. But I think the backend whose pid you have passed,
the connected client to that backend might not have superuser
privileges and if you use elevel LOG then that message will be sent to
that connected client as well and I don't think that is secure. So
can we use LOG_SERVER_ONLY so that we can prevent
it from sending to the client.

True, we should use LOG_SERVER_ONLY and not send any logs to the
client.

Thanks, agree with changing it to LOG_SERVER_ONLY.

I further tend to think that, is it correct to log queries with LOG
level when log_statement GUC is set? Or should it also be
LOG_SERVER_ONLY?

I feel it's OK to log with LOG_SERVER_ONLY since the log from
log_statement GUC would be printed already and independently.
ISTM people don't expect to log_statement GUC works even on
pg_log_current_plan(), do they?

Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

On Thu, May 13, 2021 at 3:20 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, May 13, 2021 at 3:06 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Thu, May 13, 2021 at 2:57 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Thu, May 13, 2021 at 2:44 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

+1 for the idea. I did not read the complete patch but while reading
through the patch, I noticed that you using elevel as LOG for printing
the stack trace. But I think the backend whose pid you have passed,
the connected client to that backend might not have superuser
privileges and if you use elevel LOG then that message will be sent to
that connected client as well and I don't think that is secure. So
can we use LOG_SERVER_ONLY so that we can prevent
it from sending to the client.

True, we should use LOG_SERVER_ONLY and not send any logs to the client.

I further tend to think that, is it correct to log queries with LOG
level when log_statement GUC is set? Or should it also be
LOG_SERVER_ONLY?

/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG,
(errmsg("statement: %s", query_string),
errhidestmt(true),
errdetail_execute(parsetree_list)));

What is your argument behind logging it with LOG? I mean we are
sending the signal to all the backend and some backend might have the
client who is not connected as a superuser so sending the plan to
those clients is not a good idea from a security perspective.
Anyways, LOG_SERVER_ONLY is not an exposed logging level it is used
for an internal purpose. So IMHO it should be logged with
LOG_SERVER_ONLY level.

I'm saying that - currently, queries are logged with LOG level when
the log_statement GUC is set. The queries might be sent to the
non-superuser clients. So, your point of "sending the plan to those
clients is not a good idea from a security perspective" gets violated
right? Should the log level be changed(in the below code) from "LOG"
to "LOG_SERVER_ONLY"? I think we can discuss this separately so as not
to sidetrack the main feature.

/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG,
(errmsg("statement: %s", query_string),
errhidestmt(true),
errdetail_execute(parsetree_list)));

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 13, 2021 at 4:16 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

I'm saying that - currently, queries are logged with LOG level when
the log_statement GUC is set. The queries might be sent to the
non-superuser clients. So, your point of "sending the plan to those
clients is not a good idea from a security perspective" gets violated
right? Should the log level be changed(in the below code) from "LOG"
to "LOG_SERVER_ONLY"? I think we can discuss this separately so as not
to sidetrack the main feature.

/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG,
(errmsg("statement: %s", query_string),
errhidestmt(true),
errdetail_execute(parsetree_list)));

Yes, that was my exact point, that in this particular code log with
LOG_SERVER_ONLY.

Like this.
/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG_SERVER_ONLY,
.....

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 13, 2021 at 5:14 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, May 13, 2021 at 4:16 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

I'm saying that - currently, queries are logged with LOG level when
the log_statement GUC is set. The queries might be sent to the
non-superuser clients. So, your point of "sending the plan to those
clients is not a good idea from a security perspective" gets violated
right? Should the log level be changed(in the below code) from "LOG"
to "LOG_SERVER_ONLY"? I think we can discuss this separately so as not
to sidetrack the main feature.

/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG,
(errmsg("statement: %s", query_string),
errhidestmt(true),
errdetail_execute(parsetree_list)));

Yes, that was my exact point, that in this particular code log with
LOG_SERVER_ONLY.

Like this.
/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG_SERVER_ONLY,

Agree, but let's discuss that in a separate thread.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 13, 2021 at 5:15 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Thu, May 13, 2021 at 5:14 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, May 13, 2021 at 4:16 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

I'm saying that - currently, queries are logged with LOG level when
the log_statement GUC is set. The queries might be sent to the
non-superuser clients. So, your point of "sending the plan to those
clients is not a good idea from a security perspective" gets violated
right? Should the log level be changed(in the below code) from "LOG"
to "LOG_SERVER_ONLY"? I think we can discuss this separately so as not
to sidetrack the main feature.

/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG,
(errmsg("statement: %s", query_string),
errhidestmt(true),
errdetail_execute(parsetree_list)));

Yes, that was my exact point, that in this particular code log with
LOG_SERVER_ONLY.

Like this.
/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG_SERVER_ONLY,

Agree, but let's discuss that in a separate thread.

Did not understand why separate thread? this is part of this thread
no? but anyways now everyone agreed that we will log with
LOG_SERVER_ONLY.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 13, 2021 at 5:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, May 13, 2021 at 5:15 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Thu, May 13, 2021 at 5:14 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, May 13, 2021 at 4:16 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

I'm saying that - currently, queries are logged with LOG level when
the log_statement GUC is set. The queries might be sent to the
non-superuser clients. So, your point of "sending the plan to those
clients is not a good idea from a security perspective" gets violated
right? Should the log level be changed(in the below code) from "LOG"
to "LOG_SERVER_ONLY"? I think we can discuss this separately so as not
to sidetrack the main feature.

/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG,
(errmsg("statement: %s", query_string),
errhidestmt(true),
errdetail_execute(parsetree_list)));

Yes, that was my exact point, that in this particular code log with
LOG_SERVER_ONLY.

Like this.
/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG_SERVER_ONLY,

Agree, but let's discuss that in a separate thread.

Did not understand why separate thread? this is part of this thread
no? but anyways now everyone agreed that we will log with
LOG_SERVER_ONLY.

Bharat offlist pointed to me that here he was talking about another
log that is logging the query and not specific to this patch, so let's
not discuss this point here.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On 2021-05-13 21:57, Dilip Kumar wrote:

On Thu, May 13, 2021 at 5:18 PM Dilip Kumar <dilipbalaut@gmail.com>
wrote:

On Thu, May 13, 2021 at 5:15 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Thu, May 13, 2021 at 5:14 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, May 13, 2021 at 4:16 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

I'm saying that - currently, queries are logged with LOG level when
the log_statement GUC is set. The queries might be sent to the
non-superuser clients. So, your point of "sending the plan to those
clients is not a good idea from a security perspective" gets violated
right? Should the log level be changed(in the below code) from "LOG"
to "LOG_SERVER_ONLY"? I think we can discuss this separately so as not
to sidetrack the main feature.

/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG,
(errmsg("statement: %s", query_string),
errhidestmt(true),
errdetail_execute(parsetree_list)));

Yes, that was my exact point, that in this particular code log with
LOG_SERVER_ONLY.

Like this.
/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG_SERVER_ONLY,

Agree, but let's discuss that in a separate thread.

Did not understand why separate thread? this is part of this thread
no? but anyways now everyone agreed that we will log with
LOG_SERVER_ONLY.

Modified elevel from LOG to LOG_SERVER_ONLY.

I also modified the patch to log JIT Summary and GUC settings
information.
If there is other useful information to log, I would appreciate it if
you could point it out.

Bharat offlist pointed to me that here he was talking about another
log that is logging the query and not specific to this patch, so let's
not discuss this point here.

Thanks for sharing the situation!

--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION