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@alvh.no-ip.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