Re: pg_background (and more parallelism infrastructure patches)

On Wed, Sep 10, 2014 at 2:18 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Tue, Sep 9, 2014 at 1:18 PM, Petr Jelinek <petr(at)2ndquadrant(dot)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