Re: RFC: Logging plan of the running query

On Thu, May 13, 2021 at 3:20 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Thu, May 13, 2021 at 3:06 PM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >
> > On Thu, May 13, 2021 at 2:57 PM Bharath Rupireddy
> > <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> > > On Thu, May 13, 2021 at 2:44 PM Dilip Kumar <dilipbalaut(at)gmail(dot)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