Re: Libpq support to connect to standby server as priority

Hi Peter,

Thanks for the further review, an updated patch is attached. Please
see my responses to your comments below:

On Thu, Aug 20, 2020 at 11:36 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>

>
> COMMENT (help text)
>
> The help text is probably accurate but it does seem a bit confusing still.
>
> ...
>
> 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).
>

GN RESPONSE:
I have updated the documentation, taking this view into account.

> ====
>
> 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.
>

GN RESPONSE:
My initial reasoning was that even though "prefer-secondary" is a
valid value, a GUI for target_session_attrs probably wouldn't present
that option, it would present "prefer-standby" instead (I was
imagining a drop-down menu, and it certainly wouldn't present both
"prefer-standby" and "prefer-secondary", as they are synonyms). If the
GUI did want to present the PGJDBC-compatible option values, it should
be looking at the dispsize for "Target-Server-Type" (which is 17, for
"prefer-secondary").
However, I guess there could be a number of ways to specify the option
value, even explicitly typing it into a textbox in the "database
connection dialog" that uses this information.
So in that case, I've updated the code, as you suggested, to use
dispsize=17 (for "prefer-secondary") in this case.

> ====
>
> 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"
>

GN RESPONSE: Fixed.

> ====
>
> 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.
>

GN RESPONSE:
I can't really compare and contrast the two parameters without
mentioning "target_session_attrs" here.
"target_session_attrs" implies the possibility of multiple attributes.
If the difference between the attributes is provided in separate bits
of information for each attribute, the reader may not pick up on this
subtle difference between them.

> ====
>
> 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?
>

GN RESPONSE: Yes, 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.
>

GN RESPONSE:
Blank line put back - but this appears to be because pg_indent was NOT
previously run on this code prior to me running it.

>
> 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"
>

GN RESPONSE: Fixed.

>
> 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)
>

GN RESPONSE: Changed to "else if".

>
> COMMENT (fe-connect - conn->sversion < 140000)
>
> ...
>
> 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.
>

GN RESPONSE:
I was able to refactor the code to make it a bit simpler and remove
the duplicate code block, after first adding a condition to exclude
"any".

>
> 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?
>

GN RESPONSE: I've enhanced the comment.

>
> 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!
>

GN RESPONSE:
Not deliberate, seems to have been messed up (possibly by copying
another block, to get a comment), but has now been corrected.

Regards,
Greg Nancarrow
Fujitsu Australia