RE: libpq debug log

Hello Alvaro-san, Iwata-san,

First of all, thank you Alvaro-san really a lot for your great help. I'm glad you didn't lose interest and time for this patch yet. (Iwata-san is my colleague.)

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
> That's true, but it'd require that we move PQtrace() to fe-misc.c, because
> pqTraceInit() uses definitions that are private to that file.

Ah, that was the reason for separation. Then, I'm fine with either keeping the separation, or integrating the two functions in fe-misc.c with PQuntrace() being also there. I kind of think the latter is better, because of code readability and, because tracing facility is not a connection-related reature so categorizing it as "misc" feels natural.

> > What's this code for? I think it's sufficient that PQuntrace() frees b_msg
> and PQtrace() always allocates b_msg.
>
> The point is to be able to cope with a connection that calls PQtrace() multiple
> times, with or without an intervening PQuntrace().
> We'd make no friends if we made PQtrace() crash, or leak memory if called a
> second time ...

HEAD's PQtrace() always call PQuntrace() and then re-initialize from scratch. So, if we keep that flow, the user can call PQtrace() multiple in a row.

> I think trying to apply tabs inside the message contents is going to be more
> confusing than helpful.

Agreed.

> > Plus, you may as well print the invalid message type. Why don't you do
> something like this:
> >
> > +static const char *
> > +pqGetProtocolMsgType(unsigned char c, PGCommSource commsource) {
> > + static char unknown_message[64];
> > + char *msg = NULL;
> >
> > + if (commsource == FROM_BACKEND && c <
> lengthof(protocol_message_type_b))
> > + msg = protocol_message_type_b[c];
> > + else if (commsource == FROM_FRONTEND && c <
> lengthof(protocol_message_type_f))
> > + msg = protocol_message_type_f[c];
> > +
> > + if (msg == NULL)
> > + {
> > + sprintf(unknown_message, "Unknown message %02x", c);
> > + msg = unknown_message;
> > + }
> > +
> > + return msg;
> > +}
>
> Right. I had written something like this, but in the end decided not to bother
> because it doesn't seem terribly important. You can't do exactly what you
> suggest, because it has the problem that you're returning a stack-allocated
> variable even though your stack is about to be blown away. My copy had a
> static buffer that was malloc()ed on first use (and if allocation fails, return a
> const string). Anyway, I fixed the crasher problem.

My suggestion included static qualifier to not use the stack, but it doesn't work anyway in multi-threaded applications. So I agree that we don't print the invalid message type value.

> > (18)
> > if (conn->Pfdebug)
> > - fprintf(conn->Pfdebug, "To backend> %c\n", c);
> > + pqStoreFrontendMsg(conn, LOG_BYTE1, 1);
> >
> > To match the style for backend messages with pqLogMsgByte1 etc., why
> don't you wrap the conn->Pfdebug check in the macro?
>
> I think these macros are useless and should be removed. There's more
> verbosity and confusion caused by them, than the clarity they provide.

Agreed.

> This patch needs more work still, of course.

Yes, she is updating the patch based on the feedback from you, Kirk-san, and me.

By the way, I just looked at the beginning of v12.

-void PQtrace(PGconn *conn, FILE *stream);
+void PQtrace(PGconn *conn, FILE *stream, bits32 flags);

Can we change the signature of an API function?

Iwata-san,
Please integrate Alvaro-san's patch with yours. Next week is the last week in this CF, so do your best to post the patch by next Monday or so (before Alvaro-san loses interest or time.) Then I'll review it ASAP.

Regards
Takayuki Tsunakawa