RE: libpq debug log - Mailing list pgsql-hackers
From | k.jamison@fujitsu.com |
---|---|
Subject | RE: libpq debug log |
Date | |
Msg-id | OSBPR01MB2341B376550A73AAEC05F39FEFBA9@OSBPR01MB2341.jpnprd01.prod.outlook.com Whole thread Raw |
In response to | Re: libpq debug log ('Alvaro Herrera' <alvherre@alvh.no-ip.org>) |
Responses |
RE: libpq debug log
|
List | pgsql-hackers |
On Mon, Jan 25, 2021 10:11 PM (JST), Alvaro Herrera wrote: > On 2021-Jan-25, tsunakawa.takay@fujitsu.com wrote: > > > Iwata-san, > > [...] > > > Considering these and the compilation error Kirk-san found, I'd like > > you to do more self-review before I resume this review. > > Kindly note that these errors are all mine. > > Thanks for the review > Hello, To make the CFBot happy, I fixed the compiler errors. I have not included here the change proposal to move the tracing functions to a new file: fe-trace.c or something, so I retained the changes . Maybe Iwata-san can consider the proposal. Currently, both pqTraceInit() and pqTraceUninit() are called only by one function. Summary: 1. I changed the bits32 flags to int flags 2. I assumed INT_MAX value is the former MAX_FRONTEND_MSGS I defined it to solve the compile error. Please tell if the value was wrong. 3. Fixed the doc errors 4. Added freeing of be_msg in pqTraceUninit() 5. Addressed the following comments: > (25) > + conn->fe_msg = > malloc(sizeof(offsetof(pqFrontendMessage, fields)) + > + > DEF_FE_MSGFIELDS * sizeof(pqFrontendMessageField)); > > It's incorrect that offsetof() is nested in sizeof(). See my original > review comment. I removed the initial sizeof(). > (26) > +bool > +pqTraceInit(PGconn *conn, bits32 flags) { > ... > + conn->be_msg->state = LOG_FIRST_BYTE; > + conn->be_msg->length = 0; > + } > ... > + conn->be_msg->state = LOG_FIRST_BYTE; > + conn->be_msg->length = 0; > > The former is redundant? Right. I've removed the former. > (27) > +/* > + * Deallocate frontend-message tracking memory. We only do this > +because > + * it can grow arbitrarily large, and skip it in the initial state, > +because > + * it's likely pointless. > + */ > +void > +pqTraceUninit(PGconn *conn) > +{ > + if (conn->fe_msg && > + conn->fe_msg->num_fields != DEF_FE_MSGFIELDS) > + { > + pfree(conn->fe_msg); > + conn->fe_msg = NULL; > + } > +} > > I thought this function plays the reverse role of pqTraceInit(), but > it only frees memory for frontend messages. Plus, use free() instead > of pfree(), because > malloc() is used to allocate memory. Fixed to use free(). Also added the freeing of be_msg. > (28) > +void PQtrace(PGconn *conn, FILE *stream, bits32 flags); > > bits32 can't be used because it's a data type defined in c.h, which > should not be exposed to applications. I think you can just int or something. I used int. It still works to suppress the timestamp when flags is true. In my environment, when flags is false(0): 2021-01-28 06:26:11.368 > Query 27 "COPY country TO STDOUT" 2021-01-28 06:26:11.368 > Query -1 2021-01-28 06:26:11.369 < CopyOutResponse 13 \x00 #3 #0 #0 #0 2021-01-28 06:26:11.369 < CopyDone 4 2021-01-28 06:26:11.369 < CopyDone 4 2021-01-28 06:26:11.369 < CopyDone 4 2021-01-28 06:26:11.369 < CommandComplete 11 "COPY 0" 2021-01-28 06:26:11.369 < ReadyForQuery 5 2021-01-28 06:26:11.369 > Terminate 4 2021-01-28 06:26:11.369 > Terminate -1 When flags is true(1), running the same query: > Query 27 "COPY country TO STDOUT" > Query -1 < CopyOutResponse 13 \x00 #3 #0 #0 #0 < CopyDone 4 < CopyDone 4 < CopyDone 4 < CommandComplete 11 "COPY 0" < ReadyForQuery 5 > Terminate 4 > Terminate -1 Regards, Kirk Jamison
Attachment
pgsql-hackers by date: