On Thu, Aug 20, 2020 at 10:26 AM Greg Nancarrow <gregn4...@gmail.com> wrote:
> I have updated the patch (attached) based on your comments, with
> adjustments made for additional changes based on feedback (which I
> tend to agree with) from Robert Haas and Tsunakawa san, who suggested
> read-write/read-only should be functionally different to
> primary/standby, and not just have "read-write" a synonym for
> "primary".
> I also thought it appropriate to remove "read-write", "standby" and
> "prefer-standby" from accepted values for "target_server_type"
> (instead just support "secondary" and "prefer-secondary") to match the
> similar targetServerType PGJDBC option.
> So currently have as supported option values:
>
> target_session_attrs:
> any/read-write/read-only/primary/standby(/secondary)/prefer-standby(/prefer-secondary)
> target_server_type: any/primary/secondary/prefer-secondary
>
+1 to your changes for the option values of these 2 variables.
Thanks for addressing my previous review comments in the v18 patch.
I have re-reviewed v18. Below are some additional (mostly minor)
things I noticed.
====
COMMENT (help text)
The help text is probably accurate but it does seem a bit confusing still.
Example1:
+ <para>
+ If this parameter is set to <literal>read-write</literal>,
only a connection in which
+ read-write transactions are accepted by default is considered
acceptable. To determine
+ whether the server supports read-write transactions, then if
the server is version 14
+ or greater, the support of read-write transactions is
determined by the value of the
+ <varname>transaction_read_only</varname> configuration
parameter that is reported by
+ the server upon successful connection. Otherwise if the
server is prior to version 14,
+ the query <literal>SHOW transaction_read_only</literal> will
be sent upon any successful
+ connection; if it returns <literal>on</literal>, it means the
server doesn't support
+ read-write transactions.
+ </para>
That fragment "To determine whether the server supports read-write
transactions, then" seems redundant.
Example2:
The Parameter Value descriptions seem inconsistently worded. e.g.
* "read-write" gives details about how "SHOW transaction_read_only"
can be called to decide r/w server.
* but then "read-only" doesn't mention about it
* but then "primary" does
* but then "standby" doesn't
IMO if there was some up-front paragraphs to say how different
versions calculate the r/w support and recovery mode, then all the
different parameter values can be expressed in a much simpler way and
have less repetition (e.g they can all look like the "read-only" one
does now).
e.g. I mean something similar to this (which is same wording as yours,
just rearranged a bit):
--
SERVER STATES
If the server is version 14 or greater, the support of read-write
transactions is determined by the value of the transaction_read_only
configuration parameter that is reported by the server upon successful
connection. Otherwise if the server is prior to version 14, the query
SHOW transaction_read_only will be sent upon any successful
connection; if it returns on, it means the server doesn't support
read-write transaction
The recovery mode state is determined by the value of the in_recovery
configuration parameter that is reported by the server upon successful
connection
PARAMETER VALUES
If this parameter is set to read-write, only a connection in which
read-write transactions are accepted by default is considered
acceptable.
If this parameter is set to read-only, only a connection in which
read-only transactions are accepted by default is considered
acceptable.
If this parameter is set to primary, then if the server is version 14
or greater, only a connection in which the server is not in recovery
mode is considered acceptable. Otherwise, if the server is prior to
version 14, only a connection in which read-write transactions are
accepted by default is considered acceptable.
If this parameter is set to standby, then if the server is version 14
or greater, only a connection in which the server is in recovery mode
is considered acceptable. Otherwise, if the server is prior to version
14, only a connection for which the server only supports read-only
transactions is considered acceptable.
If this parameter is set to prefer-standby, then if the server is
version 14 or greater, a connection in which the server is in recovery
mode is preferred. Otherwise, if the server is prior to version 14, a
connection for which the server only supports read-only transactions
is preferred. If no such connections can be found, then a connection
in which the server is not in recovery mode (server is version 14 or
greater) or a connection for which the server supports read-write
transactions (server is prior to version 14) will be considered
--
====
COMMENT fe-connect.c (sizeof)
- "Target-Session-Attrs", "", 11, /* sizeof("read-write") = 11 */
+ "Target-Session-Attrs", "", 15, /* sizeof("prefer-standby") = 15 */
You said changing this 15 to 17 is debatable. So I will debate it.
IIUC the dispsize is defined as /* Field size in characters for dialog */
I imagine this could be used as potential max character length of a
text input field.
Therefore, so long as "prefer-secondary" remains a valid value for
target_session_attrs then I think dispsize ought to be 17 (not 15) to
accommodate it.
Otherwise setting to 15 may be preventing dialog entry of this
perfectly valid (albeit uncommon) value.
====
COMMENT (typo)
+ /*
+ * Type of server to connect to. Possible values: "any", "primary",
+ * "prefer-secondary", "secondary" This overrides any connection type
+ * specified by target_session_attrs. This option supports a subset of the
Missing period before "This overrides"
====
COMMENT (comment)
+ /*
+ * Type of server to connect to. Possible values: "any", "primary",
+ * "prefer-secondary", "secondary" This overrides any connection type
+ * specified by target_session_attrs. This option supports a subset of the
+ * target_session_attrs option values, and its purpose is to closely
+ * reflect the similar PGJDBC targetServerType option. Note also that this
+ * option only accepts single option values, whereas in future,
+ * target_session_attrs may accept multiple session attribute values.
+ */
+ char *target_server_type;
Perhaps the part saying "... in future, target_session_attrs may
accept multiple session attribute values." more rightly belongs as a
comment for the *target_session_attrs field.
====
COMMENT (comments)
@@ -436,6 +486,8 @@ struct pg_conn
pgParameterStatus *pstatus; /* ParameterStatus data */
int client_encoding; /* encoding id */
bool std_strings; /* standard_conforming_strings */
+ bool transaction_read_only; /* transaction_read_only */
+ bool in_recovery; /* in_recovery */
Just repeating the field name does not make for a very useful comment.
Can it be improved?
====
COMMENT (blank line removal?)
@@ -540,7 +592,6 @@ struct pg_cancel
int be_key; /* key of backend --- needed for cancels */
};
-
Removal of this blank line is cleanup in some place unrelated to this patch.
====
COMMENT (typo in test comment)
+# Connect to standby1 in "prefer-ssecondary" mode with standby1,primary list.
+test_target_session_attrs($node_standby_1, $node_primary, $node_standby_1,
+ "prefer-secondary", 0);
+
Typo: "prefer-ssecondary"
====
COMMENT (fe-connect.c - suggest if/else instead of if/if)
+ /*
+ * For servers before 7.4 (which don't support read-only), if
+ * the requested type of connection is prefer-standby, then
+ * record this host index and try other specified hosts before
+ * considering it later. If the requested type of connection
+ * is read-only or standby, ignore this connection.
+ */
+
+ if (conn->requested_server_type == SERVER_TYPE_PREFER_STANDBY ||
+ conn->requested_server_type == SERVER_TYPE_READ_ONLY ||
+ conn->requested_server_type == SERVER_TYPE_STANDBY)
+ {
IIUC the only way to reach this code (because of all the previous
gotos) is when the server version is < 7.4.
So to make this more readable that "if" should ideally be "else if"
because the prior if block already says
+ if (conn->sversion >= 70400)
====
COMMENT (fe-connect - conn->sversion < 140000)
+ if (conn->sversion < 140000)
+ {
+ /*
+ * Save existing error messages across the
+ * PQsendQuery attempt. This is necessary because
+ * PQsendQuery is going to reset
+ * conn->errorMessage, so we would lose error
+ * messages related to previous hosts we have
+ * tried and failed to connect to.
+ */
+ if (!saveErrorMessage(conn, &savedMessage))
+ goto error_return;
+
+ conn->status = CONNECTION_OK;
+ if (!PQsendQuery(conn, "SHOW transaction_read_only"))
+ {
+ restoreErrorMessage(conn, &savedMessage);
+ goto error_return;
+ }
+ conn->status = CONNECTION_CHECK_WRITABLE;
+ restoreErrorMessage(conn, &savedMessage);
+ return PGRES_POLLING_READING;
+ }
I am suspicious of the duplicate code blocks for (conn->sversion < 140000).
Both appear to be doing exactly the same thing for all requests types
(excluding "any") so IMO these can be refactored into a single if
which is just beneath the check for (conn->sversion >= 70400). The
result can remove 25 lines and also be easier to read.
====
COMMENT (fe-connect.c - if comment)
+ else if ((conn->in_recovery &&
+ conn->requested_server_type == SERVER_TYPE_PRIMARY) ||
+ (!conn->in_recovery &&
+ (conn->requested_server_type == SERVER_TYPE_PREFER_STANDBY ||
+ conn->requested_server_type == SERVER_TYPE_STANDBY)))
+ {
+ /*
+ * Server is in recovery but requested primary, or
+ * server is not in recovery but requested
+ * prefer-standby/standby.
+ */
This comment does not have much value because it reads almost exactly
the same as the code it is describing.
Maybe it can be reworded to be more useful, or if not, just remove it?
====
COMMENT (fe-connect.c - CHECK_WRITABLE wrong goto?)
+ if ((readonly_server &&
+ (conn->requested_server_type == SERVER_TYPE_READ_WRITE ||
+ conn->requested_server_type == SERVER_TYPE_PRIMARY)) ||
+ (!readonly_server &&
+ (conn->requested_server_type == SERVER_TYPE_PREFER_STANDBY ||
+ conn->requested_server_type == SERVER_TYPE_READ_ONLY ||
+ conn->requested_server_type == SERVER_TYPE_STANDBY)))
{
- /* Not writable; fail this connection. */
+ if (conn->which_primary_or_rw_host == -2)
+ {
+ /*
+ * This scenario is possible only for the
+ * prefer-standby type for the next pass of the
+ * list of connections as it couldn't find any
+ * servers that are read-only.
+ */
+ goto consume_checked_target_connection;
+ }
Is this goto consume_checked_target_connection deliberate?
Previously (in the v17 patch) there was a another label, and so this
same code did goto consume_checked_write_connection;
The v17 code seems more correct than the current v18 code, which is
now jumping to a label not even in the same case block!
====
Kind Regards,
Peter Smith.
Fujitsu Australia
