Re: Wrong defeinition of pq_putmessage_noblock since 9.5

Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> At Fri, 29 Jul 2016 12:47:53 +0900, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote in <CAB7nPqSSNTroRi=zGMDxYa7PzX_VSck6hbHY6eTnBBsfYaah6A(at)mail(dot)gmail(dot)com>
>>> At Thu, 28 Jul 2016 10:46:00 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in <4313(dot)1469717160(at)sss(dot)pgh(dot)pa(dot)us>
>>> I dunno, this seems like it's doubling down on some extremely poor
>>> decisions. Why is it that you now have to flip a coin to guess whether
>>> the prefix is pq_ or socket_ for functions in this module? I would
>>> rather see that renaming reverted.

>> Yes, I agree with that. I cannot understand the intention behind
>> 2bd9e41 to rename those routines as they are now, so getting them back
>> with pg_ as prefix looks like a good idea to me.

> The many of the socket_* functions are required to be replaced
> with mq_* functions for backgroud workers. So reverting the names
> of socket_* functions should be accompanied by the need to, for
> example, changing their callers to use them explicitly via
> PqCommMethods, not hiding with macros, or renaming current pq_*
> macros to, say, pqi_. (it stands for PQ-Indirect as a tentative)
> I'm not confident on the new prefix since I'm not sure that what
> the 'pq' stands for. (Postgres Query?)

Hmm. Of course the problem with this approach is that we end up touching
a whole bunch of call sites, which sort of negates the point of having
installed a compatibility macro layer.

[ spends awhile longer looking around at this code ]

Maybe the real problem here is that the abstraction layer is so incomplete
and apparently haphazard, so that it's not very obvious where you should
use a pq_xxx name and where you should refer to socket_xxx. For some of
the functions in pqcomm.c, such as internal_flush, it's impossible to tell
which side of the abstraction boundary they're supposed to be on.
(I suspect that that particular function has simply been ignored on the
undocumented assumption that a bgworker could never call it, but that's
the kind of half-baked work that I don't find acceptable.)

I think the core work that needs to be done here is to clearly identify
each function in pqcomm.c as either above or below the PqCommMethods
abstraction boundary. Maybe we should split one set or the other out
to a new source file. In any case, the naming convention ought to
reflect that hierarchy.

I withdraw the idea that we should try to revert all these functions back
to their old names, since that would obscure the layering distinction
between socket-specific and general-usage functions. I now think that
the problem is that that distinction hasn't been drawn sharply enough.

> The set of functions in PQcommMethods doesn't seem clean. They
> are chosen arbitrarily just so that other pq_* functions used in
> parallel workers will work as expected. I suppose that it needs
> some refactoring.

Yeah, exactly.

> Any work in this area is likely 10.0 material at this point.

I'm not really happy with that, since refactoring it again will create
back-patch hazards. But I see that a lot of the mess here was created
in 9.5, which means we're probably stuck with back-patch hazards anyway.

regards, tom lane

PS: "PQ" comes from PostQUEL, I believe.