From: Greg Nancarrow <gregn4422@gmail.com>
> With the permission of the original patch author, Haribabu Kommi, I’ve
> rationalized the existing 8 patches into 3 patches, merging patches
> 1-5 and 6-7, and tidying up some documentation and code comments. I
> also rebased them to the latest PG12 source code (as of October 1,
> 2019). The patch code itself is the same, except for some version
> checks that I have updated to target the features for PG13 instead of
> PG12.
> I’ve attached the updated patches.
Thank you for taking over this patch. Your arrangement has made the patches much easier to read!
I've finished reviewing, and my comments are below. Unfortunately, 0003 failed to apply (I guess only slight
modificationis needed to apply to HEAD.) I'd like to proceed to testing when the revised patch becomes available.
(1) 0001
+ /*
+ * Requested type is prefer-read, then record this host index
+ * and try the other before considering it later. If requested
+ * type of connection is read-only, ignore this connection.
+ */
+ if (conn->requested_session_type == SESSION_TYPE_PREFER_READ ||
+ conn->requested_session_type == SESSION_TYPE_READ_ONLY)
{
This if statement seems unnecessary, because the following part at the beginning of the CONNECTION_CHECK_TARGET case
blockprecludes entering the if block. Cases other than "any" are handled first here.
if (conn->sversion >= 70400 &&
- conn->target_session_attrs != NULL &&
- strcmp(conn->target_session_attrs, "read-write") == 0)
+ conn->requested_session_type != SESSION_TYPE_ANY)
+ {
(2) 0002
-} TargetSessionAttrsType;
+} TargetSessionAttrsType;
One space after } is replaced with three tabs. I guess this is an unintentional change.
(3) 0002
+reject_checked_read_or_write_connection(PGconn *conn)
To follow the naming style of most internal functions in this file, I find it better to change the name to
rejectCheckedReadOrWriteConnection.
(4) 0003
+reject_checked_recovery_connection(PGconn *conn)
The same as the previous one.
(5) 0003
Don't we have to describe in_recovery in both or either of high-availability.sgml and config.sgml?
transaction_read_onlyis touched in the former.
Regards
Takayuki Tsunakawa