Re: Libpq support to connect to standby server as priority - Mailing list pgsql-hackers

Hi Hari-san,

I've reviewed all files.  I think I'll proceed to testing when I've reviewed the revised patch and the patch for target_server_type.

(1) patch 0001
        CONNECTION_CHECK_WRITABLE,      /* Check if we could make a writable
                                                                 * connection. */
+       CONNECTION_CHECK_TARGET,        /* Check if we have a proper target
+                                                                * connection */
        CONNECTION_CONSUME                      /* Wait for any pending message and consume
                                                                 * them. */

According to the following comment, a new enum value should be added at the end.

/*
 * Although it is okay to add to these lists, values which become unused
 * should never be removed, nor should constants be redefined - that would
 * break compatibility with existing code.
 */

(2) patch 0002
It seems to align better with the existing code to set the default value to pg_conn.requested_session_type explicitly in makeEmptyPGconn(), even if the default value is 0.  Although I feel it's redundant, other member variables do so.

(3) patch 0003
     <varname>IntervalStyle</varname> was not reported by releases before 8.4;
-    <varname>application_name</varname> was not reported by releases before 9.0.)
+    <varname>application_name</varname> was not reported by releases before 9.0
+    <varname>transaction_read_only</varname> was not reported by releases before 12.0.)

";" is missing at the end of the third line.

(4) patch 0004
-       /* Type of connection to make.  Possible values: any, read-write. */
+       /* Type of connection to make.  Possible values: any, read-write, perfer-read. */
        char       *target_session_attrs;

perfer -> prefer

(5) patch 0004
@@ -3608,6 +3691,9 @@ makeEmptyPGconn(void)
                conn = NULL;
        }

+       /* Initial value */
+       conn->read_write_host_index = -1;

The new member should be initialized earlier in this function.  Otherwise, as you can see in the above fragment, conn can be NULL in an out-of-memory case.

(6) patch 0004
Don't we add read-only as well as prefer-read, which corresponds to Slave or Secondary of PgJDBC's targetServerType?  I thought the original proposal was to add both.

(7) patch 0004
@@ -2347,6 +2367,7 @@ keep_going:                                               /* We will come back to here until there is
                                                        conn->try_next_addr = true;
                                                        goto keep_going;
                                                }
+
                                                appendPQExpBuffer(&conn->errorMessage,
                                                                                  libpq_gettext("could not create socket: %s\n"),

Is this an unintended newline addition?

(8) patch 0004
+                                               const char *type = (conn->requested_session_type == SESSION_TYPE_PREFER_READ) ?
+                                                                                       "read-only" : "writable";

I'm afraid these strings are not translatable into other languages.

(9) patch 0004
+                                               /* Record read-write host index */
+                                               if (conn->read_write_host_index == -1)
+                                                       conn->read_write_host_index = conn->whichhost;

At this point, the session can be either read-write or read-only, can't it?  Add the check "!conn->transaction_read_only" in this if condition?

(10) patch 0004
+                               if ((conn->target_session_attrs != NULL) &&
+                                          (conn->requested_session_type == SESSION_TYPE_PREFER_READ) &&
+                                          (conn->read_write_host_index != -2))

The first condition is not necessary because the second one exists.

The parenthesis surrounding each of these conditions are redundant.  It would be better to remove them for readability.  This also applies to the following part:

+                                       if (((strncmp(val, "on", 2) == 0) &&
+                                                       (conn->requested_session_type == SESSION_TYPE_READ_WRITE)) ||
+                                               ((strncmp(val, "off", 3) == 0) &&
+                                                       (conn->requested_session_type == SESSION_TYPE_PREFER_READ) &&
+                                                       (conn->read_write_host_index != -2)))