On Wed, Sep 10, 2014 at 2:18 AM, Robert Haas <robertmh...@gmail.com> wrote: > > On Tue, Sep 9, 2014 at 1:18 PM, Petr Jelinek <p...@2ndquadrant.com> wrote: > >> I think that's completely wrong. As the patch series demonstrates, > >> it's not limited to propagating ErrorResponse and NoticeResponse. It > >> can also propagate NotifyResponse and RowDescription and DataRow and > >> anything else that comes along. We are not just propagating errors; > >> we are propagating all protocol messages of whatever type. So tying > >> it to elog specifically is not right. > > > > Oh in that case, I think what Andres proposed is actually quite good. I know > > the hook works fine it just seems like using somewhat hackish solution to > > save 20 lines of code. > > If it's 20 lines of code, I'm probably fine to go that way. Let's see > if we can figure out what those 20 lines look like. > > libpq.h exports 29 functions that do a variety of different things. > Of those, 20 are in pqcomm.c and the others are in be-secure.c. I > presume that pluggability for the latter group, if needed at all, is a > separate project. The remaining ones break down like this: > > - StreamServerPort, StreamConnection, StreamClose, and > TouchSocketFiles are intended to be called only from the postmaster, > to set up and tear down the listening socket and individual > connections. Presumably this is not what we care about here. > - pq_getbytes(), pq_getstring(), pq_getmessage(), pq_getbyte(), > pq_peekbyte(), and pq_getbyte_if_available() handle *reading* data > from the socket. Since you previously agreed that we didn't need to > build two-way communication on top of this, I would thank that would > mean that these don't need to be pluggable either. But maybe I'm > wrong. > - The remaining functions are pq_init(), pq_comm_reset(), pq_flush(), > pq_flush_if_writable(), pq_is_send_pending(), pq_putmessage(), > pq_putmessage_noblock(), pq_startcopyout(), and pq_endcopyout(). > These are the ones that I think are potentially interesting. > > I didn't choose to provide hooks for all of these in the submitted > patch because they're not all needed for I want to do here: > pq_startcopyout() and pq_endcopyout() are only needed for V2 protocol > support, which did not interest me (nor did COPY, really); > pq_putmessage_noblock(), pq_flush_if_writable(), and > pq_is_send_pending() are only used for the walsender protocol, which > doesn't seem useful to redirect to a non-socket; and I just didn't > happen to have any use for pq_init() or pq_comm_reset(). Hence what I > ended up with. > > But, I could revisit that. Suppose I provide a structure with 10 > function pointers for all ten of those functions, or maybe 9 since > pq_init() is called so early that it's not likely we'll have control > to put the hooks in place before that point, and anyway whatever code > installs the hooks can do its own initialization then.
Can we use pq_init() to install function pointers? Let us say that it will take COMM_METHOD (tcp, shm_mq) as input and then install function pointers based on communication method. We can call this from main function of bgworker (in case of patch from pg_background_worker_main()) with COMM_METHOD as shm_mq and BackendInitialize() will pass it as tcp. > Then make a > global variable like pqSendMethods and #define pq_comm_reset() to be > pqSendMethods->comm_reset(), pflush() to be pqSendMethods->flush(), > and so on for all 9 or 10 methods. Then the pqmq code could just > change pqSendMethods to point to its own method structure instead of > the default one. Would that address the concern this concern? It's > more than 20 lines of code, but it's not TOO bad. This idea seems to be better than directly using hooks, however I don't see any harm in defining pqReceiveMethods for get API's as well because it can make the whole layer extendable. Having said that I think as currently there is no usage of it, so we can leave it for another patch as well. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com