Re: Libpq support to connect to standby server as priority

On Mon, Feb 25, 2019 at 11:38 AM Tsunakawa, Takayuki <
tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com> wrote:

> 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.
>
>
Thanks for the review.

>
> (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.
> */
>

fixed.

>
>
> (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.
>
>
fixed.

>
> (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.
>
>
fixed.

>
> (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
>
>
fixed.

>
> (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.
>
>
fixed.

>
> (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.
>
>
Added read-only option.

>
> (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?
>
>
removed.

>
> (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.
>
>
OK. I added two separate error message prepare statements for both
read-write and read-only
so, it shouldn't be a problem.

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

Yes, fixed.

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

fixed.

Attached are the updated patches.
The target_server_type option yet to be implemented.

Regards,
Haribabu Kommi
Fujitsu Australia