Rotten parts of src/backend/replication/README

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Rotten parts of src/backend/replication/README

Michael Paquier-2
Hi all,

The first part of src/backend/replication/README lists all the APIs
usable for a WAL receiver, but these have aged and lost track of most
changes that happened over the years.  Four functions are listed in
the README, with incorrect signatures and many others are missing:
- walrcv_connect()
- walrcv_receive()
- walrcv_send()
- walrcv_disconnect()

I think that we should clean up that.  And as it seems to me that
nobody really remembers to update this README, I would suggest to
update the first section of the README to refer to walreceiver.h for
details about each function, then move the existing API descriptions
from the README to walreceiver.h (while fixing the incomplete
descriptions of course).  This way, if a new function is added or if
an existing function is changed, it is going to be hard to miss an
update of the function descriptions.

Any thoughts?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Rotten parts of src/backend/replication/README

akapila
On Sat, May 2, 2020 at 8:16 AM Michael Paquier <[hidden email]> wrote:

>
> Hi all,
>
> The first part of src/backend/replication/README lists all the APIs
> usable for a WAL receiver, but these have aged and lost track of most
> changes that happened over the years.  Four functions are listed in
> the README, with incorrect signatures and many others are missing:
> - walrcv_connect()
> - walrcv_receive()
> - walrcv_send()
> - walrcv_disconnect()
>
> I think that we should clean up that.

+1.

>  And as it seems to me that
> nobody really remembers to update this README, I would suggest to
> update the first section of the README to refer to walreceiver.h for
> details about each function, then move the existing API descriptions
> from the README to walreceiver.h (while fixing the incomplete
> descriptions of course).  This way, if a new function is added or if
> an existing function is changed, it is going to be hard to miss an
> update of the function descriptions.
>

I think in README we can have a general description of the module and
maybe at the broad level how different APIs can help to achieve the
required functionality and then for API description we can refer to
.h/.c.  The detailed description of APIs should be where those APIs
are defined.  The header file can contain some generic description.
The detailed description I am referring to is below in the README:
"Retrieve any message available without blocking through the
connection.  If a message was successfully read, returns its length.
If the connection is closed, returns -1.  Otherwise returns 0 to
indicate that no data is available, and sets *wait_fd to a socket
descriptor which can be waited on before trying again.  On success, a
pointer to the message payload is stored in *buffer. The returned
buffer is valid until the next call to walrcv_* functions, and the
caller should not attempt to free it."

I think having such a description near the actual definition helps in
keeping it updated whenever we change the function.


--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Rotten parts of src/backend/replication/README

Michael Paquier-2
On Sat, May 02, 2020 at 04:54:32PM +0530, Amit Kapila wrote:

> I think in README we can have a general description of the module and
> maybe at the broad level how different APIs can help to achieve the
> required functionality and then for API description we can refer to
> .h/.c.  The detailed description of APIs should be where those APIs
> are defined.  The header file can contain some generic description.
> The detailed description I am referring to is below in the README:
> "Retrieve any message available without blocking through the
> connection.  If a message was successfully read, returns its length.
> If the connection is closed, returns -1.  Otherwise returns 0 to
> indicate that no data is available, and sets *wait_fd to a socket
> descriptor which can be waited on before trying again.  On success, a
> pointer to the message payload is stored in *buffer. The returned
> buffer is valid until the next call to walrcv_* functions, and the
> caller should not attempt to free it."
>
> I think having such a description near the actual definition helps in
> keeping it updated whenever we change the function.
Yeah.  The years have visibly proved that the README had updates when
it came to the general descriptions of the libpqwalreceiver interface,
but that we had better consolidate the header to give some
documentation to whoever plays with this interface.  Attached is a
patch to address all that, where I simplified the README and added
some description to all the callbacks.  Thoughts are welcome.  I'll
add that to the next CF.  Now I don't see any actual problems in
getting that on HEAD before v13 forks.  But let's gather more opinions
first.
--
Michael

repl-readme-v1.patch (7K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Rotten parts of src/backend/replication/README

Peter Eisentraut-6
On 2020-05-04 07:50, Michael Paquier wrote:
> Yeah.  The years have visibly proved that the README had updates when
> it came to the general descriptions of the libpqwalreceiver interface,
> but that we had better consolidate the header to give some
> documentation to whoever plays with this interface.  Attached is a
> patch to address all that, where I simplified the README and added
> some description to all the callbacks.  Thoughts are welcome.  I'll
> add that to the next CF.  Now I don't see any actual problems in
> getting that on HEAD before v13 forks.  But let's gather more opinions
> first.

This patch looks good to me.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Rotten parts of src/backend/replication/README

Michael Paquier-2
On Tue, Jun 30, 2020 at 10:11:04AM +0200, Peter Eisentraut wrote:

> On 2020-05-04 07:50, Michael Paquier wrote:
>> Yeah.  The years have visibly proved that the README had updates when
>> it came to the general descriptions of the libpqwalreceiver interface,
>> but that we had better consolidate the header to give some
>> documentation to whoever plays with this interface.  Attached is a
>> patch to address all that, where I simplified the README and added
>> some description to all the callbacks.  Thoughts are welcome.  I'll
>> add that to the next CF.  Now I don't see any actual problems in
>> getting that on HEAD before v13 forks.  But let's gather more opinions
>> first.
>
> This patch looks good to me.
Thanks for the review, Peter.  After an extra read, the description
of walrcv_create_slot_fn was not complete (missed the end of the
sentence to say that NULL is returned for a physical slot) and had a
grammar mistake.  So I have fixed this part, and applied the patch on
HEAD.  Perhaps things could be improved further more, so if anybody
has any suggestion please feel free.
--
Michael

signature.asc (849 bytes) Download Attachment