RE: libpq debug log

On Mon, Jan 25, 2021 10:11 PM (JST), Alvaro Herrera wrote:
> On 2021-Jan-25, tsunakawa(dot)takay(at)fujitsu(dot)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