Rotten parts of src/backend/replication/README
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
On Sat, May 2, 2020 at 8:16 AM Michael Paquier <michael@paquier.xyz> 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
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
Attachments:
repl-readme-v1.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/include/replication/walreceiver.h b/src/include/replication/walreceiver.h
index f1aa6e9977..6b87072464 100644
--- a/src/include/replication/walreceiver.h
+++ b/src/include/replication/walreceiver.h
@@ -213,39 +213,163 @@ typedef struct WalRcvExecResult
TupleDesc tupledesc;
} WalRcvExecResult;
-/* libpqwalreceiver hooks */
-typedef WalReceiverConn *(*walrcv_connect_fn) (const char *conninfo, bool logical,
+/* WAL receiver - libpqwalreceiver hooks */
+
+/*
+ * walrcv_connect_fn
+ *
+ * Establish connection to a cluster. 'logical' is true if the
+ * connection is logical, and false if the connection is physical.
+ * 'appname' is a name associated to the connection, to use for example
+ * with fallback_application_name or application_name. Returns the
+ * details about the connection established, as defined by
+ * WalReceiverConn for each WAL receiver module. On error, NULL is
+ * returned with 'err' including the error generated.
+ */
+typedef WalReceiverConn *(*walrcv_connect_fn) (const char *conninfo,
+ bool logical,
const char *appname,
char **err);
+
+/*
+ * walrcv_check_conninfo_fn
+ *
+ * Parse and validate the connection string given as of 'conninfo'.
+ */
typedef void (*walrcv_check_conninfo_fn) (const char *conninfo);
+
+/*
+ * walrcv_get_conninfo_fn
+ *
+ * Returns a user-displayable conninfo string. Note that any
+ * security-sensitive fields should be obfuscated.
+ */
typedef char *(*walrcv_get_conninfo_fn) (WalReceiverConn *conn);
+
+/*
+ * walrcv_get_senderinfo_fn
+ *
+ * Provide information of the WAL sender this WAL receiver is connected
+ * to, as of 'sender_host' for the host of the sender and 'sender_port'
+ * for its port.
+ */
typedef void (*walrcv_get_senderinfo_fn) (WalReceiverConn *conn,
char **sender_host,
int *sender_port);
+
+/*
+ * walrcv_identify_system_fn
+ *
+ * Run IDENTIFY_SYSTEM on the cluster connected to and validate the
+ * identity of the cluster. Returns the system ID of the cluster
+ * connected to. 'primary_tli' is the timeline ID of the sender.
+ */
typedef char *(*walrcv_identify_system_fn) (WalReceiverConn *conn,
TimeLineID *primary_tli);
+
+/*
+ * walrcv_server_version_fn
+ *
+ * Returns the version number of the cluster connected to.
+ */
typedef int (*walrcv_server_version_fn) (WalReceiverConn *conn);
+
+/*
+ * walrcv_readtimelinehistoryfile_fn
+ *
+ * Fetch from cluster the timeline history file for timeline 'tli'.
+ * Returns the name of the timeline history file as of 'filename', its
+ * contents as of 'content' and its 'size'.
+ */
typedef void (*walrcv_readtimelinehistoryfile_fn) (WalReceiverConn *conn,
TimeLineID tli,
char **filename,
- char **content, int *size);
+ char **content,
+ int *size);
+
+/*
+ * walrcv_startstreaming_fn
+ *
+ * Start streaming WAL data from given streaming options. Returns true
+ * if the connection has switched successfully to copy-both mode and false
+ * if the server received the command and executed it successfully, but
+ * didn't switch to copy-mode.
+ */
typedef bool (*walrcv_startstreaming_fn) (WalReceiverConn *conn,
const WalRcvStreamOptions *options);
+
+/*
+ * walrcv_endstreaming_fn
+ *
+ * Stop streaming of WAL data. Returns the next timeline ID of the cluster
+ * connected to in 'next_tli', or 0 if there was no report.
+ */
typedef void (*walrcv_endstreaming_fn) (WalReceiverConn *conn,
TimeLineID *next_tli);
-typedef int (*walrcv_receive_fn) (WalReceiverConn *conn, char **buffer,
+
+/*
+ * walrcv_receive_fn
+ *
+ * Receive a message available from the WAL stream. 'buffer' is a pointer
+ * to a buffer holding the message received. Returns the length of the data,
+ * 0 if no data is available yet ('wait_fd' is a socket descriptor which can
+ * be waited on before a retry), and -1 if the cluster ended the COPY.
+ */
+typedef int (*walrcv_receive_fn) (WalReceiverConn *conn,
+ char **buffer,
pgsocket *wait_fd);
-typedef void (*walrcv_send_fn) (WalReceiverConn *conn, const char *buffer,
+
+/*
+ * walrcv_send_fn
+ *
+ * Send a message of size 'nbytes' to the WAL stream with 'buffer' as
+ * contents.
+ */
+typedef void (*walrcv_send_fn) (WalReceiverConn *conn,
+ const char *buffer,
int nbytes);
+
+/*
+ * walrcv_create_slot_fn
+ *
+ * Create a new replication slot named 'slotname'. 'temporary' defines
+ * if the slot is temporary. 'snapshot_action' defined the behavior wanted
+ * for an exported snapshot (see replication protocol for more details).
+ * 'lsn' includes the LSN position at which the created slot became
+ * consistent. Returns the name of the exported snapshot for a logical
+ * slot, and NULL
+ */
typedef char *(*walrcv_create_slot_fn) (WalReceiverConn *conn,
- const char *slotname, bool temporary,
+ const char *slotname,
+ bool temporary,
CRSSnapshotAction snapshot_action,
XLogRecPtr *lsn);
+
+/*
+ * walrcv_get_backend_pid_fn
+ *
+ * Returns the PID of the remote backend process.
+ */
typedef pid_t (*walrcv_get_backend_pid_fn) (WalReceiverConn *conn);
+
+/*
+ * walrcv_exec_fn
+ *
+ * Send generic queries (and commands) to the remote cluster. 'nRetTypes'
+ * is the expected number of returned attributes, and 'retTypes' an array
+ * including their type OIDs. Returns the status of the execution and
+ * tuples if any.
+ */
typedef WalRcvExecResult *(*walrcv_exec_fn) (WalReceiverConn *conn,
const char *query,
const int nRetTypes,
const Oid *retTypes);
+
+/*
+ * walrcv_disconnect_fn
+ *
+ * Disconnect with the cluster.
+ */
typedef void (*walrcv_disconnect_fn) (WalReceiverConn *conn);
typedef struct WalReceiverFunctionsType
diff --git a/src/backend/replication/README b/src/backend/replication/README
index 8ccdd86e74..eae6ca729f 100644
--- a/src/backend/replication/README
+++ b/src/backend/replication/README
@@ -8,33 +8,8 @@ the primary server, receiving WAL files and sending messages, is loaded
dynamically to avoid having to link the main server binary with libpq.
The dynamically loaded module is in libpqwalreceiver subdirectory.
-The dynamically loaded module implements four functions:
-
-
-bool walrcv_connect(char *conninfo, XLogRecPtr startpoint)
-
-Establish connection to the primary, and starts streaming from 'startpoint'.
-Returns true on success.
-
-int walrcv_receive(char **buffer, pgsocket *wait_fd)
-
-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.
-
-void walrcv_send(const char *buffer, int nbytes)
-
-Send a message to XLOG stream.
-
-void walrcv_disconnect(void);
-
-Disconnect.
-
+The dynamically loaded module implements a set of functions with details
+about each one of them provided in src/include/replication/walreceiver.h.
This API should be considered internal at the moment, but we could open it
up for 3rd party replacements of libpqwalreceiver in the future, allowing
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
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