Re: [HACKERS] LIBPQ patches ...

* Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> [000108 14:56] wrote:
> The Hermit Hacker <scrappy(at)hub(dot)org> writes:
> > Does anyone have anything against me applying this to the current source
> > tree?
>
> I'm not particularly comfortable with it --- it looks like the semantics
> need more careful thought, particularly concerning when the output buffer
> gets flushed and what happens if we can't send data right away.

Could you be more specific? My patches address the fact that
although there is work to make libpq non-blocking you can easily
block while sending large queries expecially because of the select()
that is done in pqFlush().

The problem is that libpq doesn't reserve space in the send buffer
and will just block if waiting for the socket to the backend to
drain. This needs to be fixed if libpq is trully going to offer
non-blocking behavior.

Unless you reserve the space in the buffer you have to block
otherwise if you abort (so as not to block) then libpq may have
sent a partial query down the pipe or just buffered part of some
data you've sent to the backend. At this point you will be out of sync
with the backend.

If you are in 'normal mode' (blocking) then the behavior shouldn't
be any different, if you are non-blocking then if you attempt to
send data and it's not possible you'll get an error without
potentially sending a partial line to the backend.

> The
> insertion of a pqFlush into PQconsumeInput, in particular, looks like
> an ill-thought-out hack that could break some applications.

I think I agree, the code I was using would attempt an PQconsumeInput()
before doing a PQendcopy(), there could be data in the send buffer
that would make PQconsumeInput() never succeed hence the need for a
flush.

I'm going to try it without the PQconsumeInput() before the PQendcopy()
my modifications for PQendcopy() should make it non-blocking safe.
but in the meanwhile here's my (probably wrong) reasoning behind this
'hack':

No, IMHO it's needed, the problem is that there may be data
in the send buffer that hasn't been sent yet, it could be
part of a request to the backend that you are explicitly
waiting for a result from.

This can happen when doing a COPY into the database.

What happens is that you send data, then when you send the
'end copy' it can get buffered, then you loop forever
attempting to read a result for a query that was never
sent.

In regards to it breaking applications, the send buffer
should be opaque to the libpq user, libpq never has offered
a truly non-blocking api, and even when using non-blocking
the flush will fail if it can't be done and PQconsumeInput()
will error out accordingly.

Old applications can be snagged by the Flush since in theory
PQconsumeInput shouldn't block, however I'm not sure if
there's a real workaround for this except

1.. saving the blocking status of the connection,
2.. setting it non-blocking and attempting a flush and then
3.. restoring the blocking status.

It seems that old applications can break (looping on an
unsent line) regardless because of the not-flushed-query
problem.

If you can figure an occasion where this might actually happen
(with the exception of my accidentaly abuse of libpq) then it
may need to be revisited.

I'll get back to you guys on the PQendcopy before PQconsumeInput
tests.

> I also object strongly to the lack of documentation. Patches that
> change public APIs and come without doco updates should be rejected
> out of hand, IMNSHO. Keeping the documentation up to date should
> not be considered optional --- especially not when you're talking
> about something that makes subtle and pervasive changes to library
> behavior.

I agree with you about the documentation issues, I will try to add
some documentation to the patches.

I think I can also take out the visibility of the PQflush() function
as normal applications really shouldn't need it.

How do you feel about the explicit PQsetnonblocking and PQisnonblocking
functions that I added as well as the additional field 'nonblocking'
added to PGconn? IMO the user shouldn't set the socket non-blocking
without informing the library about it otherwise it gets really ugly
because we have to constantly poll the socket's flags to make sure we
DTRT.

I also apologize for my not indented patches, is there a way to indent
according to postgresql standards on a FreeBSD system? The patches
for pgindent are a bit out of date and I get floating point exceptions
when attempting to pgindent.

Thanks for the feedback.
-Alfred