On Mon, Jan 25, 2021 10:11 PM (JST), Alvaro Herrera wrote: > On 2021-Jan-25, [email protected] 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
v14-0001-libpq-trace.patch
Description: v14-0001-libpq-trace.patch
