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 <alvhe...@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
