Re: Add PQsendSyncMessage() to libpq

On Fri, 28 Apr 2023 at 14:07, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I wonder whether this is the naming that we want. The two names are
> significantly different. Something like PQpipelineSendSync() would be
> more similar.
>
> I also wonder, really even more, whether it would be better to do
> something like PQpipelinePutSync(PGconn *conn, bool flush) with
> PQpipelineSync(conn) just meaning PQpipelinePutSync(conn, true). We're
> basically using the function name as a Boolean parameter to select the
> behavior, which is fine if you only have one parameter and it's a
> Boolean, but it's obviously unworkable if you have say 3 Boolean
> parameters because you don't want 8 different functions, and what if
> you need an integer parameter for some reason?

On Wed, 3 May 2023 at 12:04, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> I agree that adding a flag is the way to go, since it improve chances
> that we won't end up with ten different functions in case we decide to
> have eight other behaviors. One more function and we're done. And
> while I can't think of any use for a future flag, we (I) already didn't
> of this one either, so let's not make the same mistake.

On Sat, 29 Apr 2023 at 18:07, Anton Kirilov <antonvkirilov(at)gmail(dot)com> wrote:
> The reason is that the function is modeled after PQsendFlushRequest(),
> since it felt closer to what I was trying to achieve, i.e. appending a
> protocol message to the output buffer without doing any actual I/O
> operations.

Sorry for being late to the party, but I think the current API naming
and the flag argument don't fit well with the current libpq API that
we have. I much prefer something similar to the original version of
the patch.

I think this function should be named something with the "PQsend"
prefix since that's the way we name all our public async message
sending functions in libpq. The "Put" word we only use in internal
libpq functions, so I feel it has no place in the external API
surface. My proposal would be to call the function PQsendPipelineSync
(i.e. having the PQsend prefix while still looking similar to the
existing PQpipelineSync).

Also I think the flag argument is completely unnecessary. I understand
the argument that we didn't foresee the need for this non-flushing
behaviour either, and the follow up reasoning that we thus should add
a flag for future things we didn't forsee. But I think it's looking at
the situation from the wrong direction. Instead of looking at it as
adding another version of our current PQpipelineSync API, we should
look at it as an addition to our current list of PQsend functions for
a new packet type. And none of those PQsend functions ever needed a
flag. Which makes sense, because they are the lowest level building
blocks that make sense from a user perspective: They send a message
type over the socket and don't do anything else. And if the assumption
that this is the lowest level building block is wrong, then it will
almost certainly be wrong for all other PQsend functions too. And thus
we'll need a solution that fits for all of them.

Finally, I have one suggestion for a behavioural change: I think the
function should still call pqPipelineFlush, just like all of our other
PQsend functions (except PQsendFlushRequest, but that seems like an
oversight there too).