Hi Greg,
I have spent some time reading this discussion thread, and doing a code review
of the latest (v17-0001) patch.
Below are my review comments; some are trivial, others not so much.
====
GENERAL COMMENT 1 ("any")
"any" should be included as valid option for target_server_type.
IIUC target_server_type was added mostly to have better alignment with JDBC
options.
Both Vladimir [1] and Dave [2] already said that JDBC does have an "any" option.
[1] -
https://www.postgresql.org/message-id/CAB%3DJe-FwOVE%3D8gR1UDDZRnWZR65fRG40e8zW_U_6mnUqbce68g%40mail.gmail.com
[2] -
https://www.postgresql.org/message-id/CADK3HHJ9316ji7L-97cJBY%3Dwp4E3ddPMn8XdkNz6j8d9u0OhmQ%40mail.gmail.com
Furthermore, the fe-connect.c function makeEmptyPGConn sets default:
+ conn->requested_server_type = SERVER_TYPE_ANY;
This means the default type of target_server_type is "any".
Since this is default, it should also be possible to assign the same value to
explicitly.
(Parts of the v17 patch affected by this are itemised below)
====
GENERAL COMMENT 2 (Removal of pg_is_in_recovery)
Around 22/3/2019 Hari added a lot of pg_is_in_recovery code in his patch 0006
[1]
[1] -
https://www.postgresql.org/message-id/CAJrrPGd4YeA%2BN%3DxC%2B1XPVoGzMCATJZY4irVQEJ6i0aPqorUi7g%40mail.gmail.com
Much later IIUC the latest v17 patch has taken onboard the recommendation from
Alvaro and removed all that code:
"I would discard the whole thing about checking "SELECT pg_is_in_recovery()""
[2]
[2] -
https://www.postgresql.org/message-id/20191227130828.GA21647%40alvherre.pgsql
However, it seems that not ALL parts of the original code got cleanly removed
in v17.
There are a number of references to CONNECTION_CHECK_RECOVERY and
pg_is_in_recovery still lurking.
(Parts of the v17 patch affected by this are itemised below)
====
COMMENT libpq.sgml (para blocks)
+ <para>
The v17 patch for target_session_attrs and target_server_type help is currently
using <para> blocks for each of the possible option values.
This format is inconsistent document style with other variables in this SGML.
Other places are using sub-lists for option values. e.g. look at "six modes" of
sslmode.
====
COMMENT libpq.sgml (cut/paste parameter description)
I don't think that target_server_type help should be just a cut/paste duplicate
of target_session_attrs. It is confusing because it leaves the reader doubting
the purpose of having such a duplication.
Suggest to simplify the target_server_type help like as follows:
--
target_server_type
The purpose of this parameter is to reflect the similar PGJDBC targetServerType.
The supported options are same as target_session_attrs.
This parameter overrides any connection type specified by target_session_attrs.
--
====
COMMENT libpq.sgml (pg_is_in_recovery)
(As noted in GENERAL COMMENT 2 there are still residual references to
pg_is_in_recovery)
+ <para>
+ If this parameter is set to <literal>standby</literal>, only a
connection in which
+ the server is in recovery mode is considered acceptable. If the server
is prior to version 14,
+ the query <literal>SELECT pg_is_in_recovery()</literal> will be sent
upon any successful
+ connection; if it returns <literal>t</literal>, it means the server is
in recovery mode.
+ </para>
Suggest change to:
--
If this parameter is set to <literal>standby</literal>, only a connection in
which the server is in recovery mode is considered acceptable. The recovery
mode state is determined by the value of the in_recovery configuration
parameter that is reported by the server upon successful connection. Otherwise,
if the server is prior to version 14, only a connection in which read-write
transactions are not accepted by default is considered acceptable. To determine
whether the server supports read-write transactions, 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 transactions.
--
====
COMMENT libpq.sgml (Oxford comma)
+ <varname>integer_datetimes</varname>,
+ <varname>standard_conforming_strings</varname> and
+ <varname>in_recovery</varname>.
Previously there was an Oxford comma (e.g. before the "and"). Now there isn't.
The v17 patch should not alter the previous listing style.
====
COMMENT protocol.sgml (Oxford comma)
+ <varname>integer_datetimes</varname>,
+ <varname>standard_conforming_strings</varname> and
+ <varname>in_recovery</varname>.
Previously there was an Oxford comma (e.g. before the "and"). Now there isn't.
The v17 patch should not alter the previous listing style.
====
QUESTION standby.c - SendRecoveryExitSignal
+/*
+ * SendRecoveryExitSignal
+ * Signal backends that the server has exited recovery mode.
+ */
+void
+SendRecoveryExitSignal(void)
+{
+ SendSignalToAllBackends(PROCSIG_RECOVERY_EXIT);
+}
I wonder if this function is really necessary?
IIUC the SendRecoveryExitSignal is only called from one place (xlog.c).
Why not just call SendSignalToAllBackends directly from there and remove this
extra layer?
====
COMMENT postgres.c (signal comment)
+ /* signal that work needs to be done */
+ recoveryExitInterruptPending = true;
Suggest change comment to say:
/* flag that work needs to be done */
====
COMMENT fe-connect.c (sizeof)
- "Target-Session-Attrs", "", 11, /* sizeof("read-write") = 11 */
+ "Target-Session-Attrs", "", 15, /* sizeof("prefer-standby") =
15 */
According to the SGML "prefer-secondary" is also an acceptable value for
target_session_attrs, so the display field width should be 17 /*
sizeof("prefer-secondary") */ not 15.
====
COMMENT fe-connect.c (CONNECTION_CHECK_RECOVERY)
@@ -2310,6 +2461,7 @@ PQconnectPoll(PGconn *conn)
case CONNECTION_CHECK_WRITABLE:
case CONNECTION_CONSUME:
case CONNECTION_GSS_STARTUP:
+ case CONNECTION_CHECK_RECOVERY:
break;
(As noted in GENERAL COMMENT 2 there are still residual references to
pg_is_in_recovery)
Probably this CONNECTION_CHECK_RECOVERY case should be removed.
====
COMMENT fe-connect.c - function validateAndRecordTargetServerType
As noted in GENERAL COMMENT 1, I suggest "any" needs to be included in this
function as a valid option.
====
COMMENT fe-connect.c (target_session_attrs validation)
@@ -1396,8 +1425,9 @@ connectOptions2(PGconn *conn)
*/
if (conn->target_session_attrs)
{
- if (strcmp(conn->target_session_attrs, "any") != 0
- && strcmp(conn->target_session_attrs, "read-write") !=
0)
+ if (strcmp(conn->target_session_attrs, "any") == 0)
+ conn->requested_server_type = SERVER_TYPE_ANY;
+ else if
(!validateAndRecordTargetServerType(conn->target_session_attrs,
&conn->requested_server_type))
I suggest introducing a 2nd function for target_session_attrs (e.g.
validateAndRecordTargetSessionAttrs).
Even though these parameters are functionally the same today, in future they
may not be [1].
[1] -
https://www.postgresql.org/message-id/20200109152539.GA29017%40alvherre.pgsql
Regardless, the special "any" handling can be removed from here because (from
GENERAL COMMENT 1) the validateAndRecordTargetServerType should now accept
"any".
====
COMMENT fe-connect.c (message typo)
Found an existing typo, unrelated to the v17 patch.
"target_settion_attrs", --> "target_session_attrs",
====
COMMENT fe-connect.c (libpq_gettext)
+ printfPQExpBuffer(&conn->errorMessage,
+
libpq_gettext("invalid target_server_type value: \"%s\"\n"),
+
conn->target_server_type);
The parameter name "target_server_type" should be separated from the format
string as "%s", the same as is done by the libpq_gettext of the preceding code.
====
COMMENT fe-connect.c (indentation)
+ goto error_return;
+ }
}
+ else
conn->whichhost++;
Bad indentation of the else's statement.
====
COMMENT fe-connect.c (if/else complexity)
+ 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)))
+ {
TBH I was unable to read this code without first drawing up a matrix of
combinations to deduce what was going on.
It should not be so inscrutable.
Suggestion1:
Consider putting a large comment at the top of this CONNECTION_CHECK_TARGET to
give the overview what this code is trying to acheive.
e.g. something like this:
---
Mode |in_recovery |version < 7.4 |version < 14
|version >= 14
---------------+------------+-------------------+---------------------------+-------------
ANY |NA |OK |OK |OK
PRIMARY |true |OK |SHOW transaction_read_only
|keep_going
PRIMARY |false |OK |SHOW transaction_read_only |OK
PREFER_STANDBY |true |keep_going (or -2) |SHOW transaction_read_only
|OK
PREFER_STANDBY |false |keep_going (or -2) |SHOW transaction_read_only
|keep_going (or -2)
STANDBY |true |keep_going |SHOW transaction_read_only
|OK
STANDBY |false |keep_going |SHOW transaction_read_only
|keep_going
---
Suggestion2:
Consider to separate out the requested_server_type cases instead of trying to
hand everything in the same else block. The code may be a bit longer, but by
aligning it more closely with the SGML documentation it can be made easier to
understand.
e.g. something like this:
---
if (conn->requested_server_type == SERVER_TYPE_PRIMARY) {
/* If not-in-recovery, reject, else OK. */
if (conn->in_recovery) {
rejectCheckedRecoveryConnection(conn);
goto keep_going;
}
goto consume_checked_target_connection;
}
if (conn->requested_server_type == SERVER_TYPE_STANDBY) {
/* Only a connection in recovery mode is acceptable. */
if (!conn->in_recovery) {
rejectCheckedRecoveryConnection(conn);
goto keep_going;
}
goto consume_checked_target_connection;
}
if (conn->requested_server_type == SERVER_TYPE_PREFER_STANDBY) {
/* A connection in recovery mode is preferred. */
if (conn->in_recovery)
goto consume_checked_target_connection;
/*
* The following 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 in recovery.
*/
if (conn->which_rw_host == -2)
goto consume_checked_target_connection;
/* reject function below remembers this r/w host index in case it is
needed later */
rejectCheckedRecoveryConnection(conn);
goto keep_going;
}
---
====
COMMENT fe-connect.c (case CONNECTION_CHECK_RECOVERY)
(As noted in GENERAL COMMENT 2 there are still residual references to
pg_is_in_recovery)
v17 patch has removed the previous call to pg_is_in_recovery.
IIUC this means that there is currently no way for the remaining
CONNECTION_CHECK_RECOVERY case to even be executed.
If I am correct, then a significant slab of code (~100 lines) can be deleted.
See case CONNECTION_CHECK_RECOVERY (lines ~ 4007 thru 4110)
====
COMMENT fe-connect.c - function freePGConn (missing free?)
There is code to free(conn->target_session_attrs), but there is no code to free
target_server_type.
Appears to be accidental omission.
====
COMMENT fe-exec.c (altered comment)
- * Special hacks: remember client_encoding and
+ * Special hacks: remember client_encoding, and
A comma was added.
Suggest avoid altering comments not directly related to the v17 patch logic.
====
COMMENT libpq-fe.h (CONNECTION_CHECK_RECOVERY)
+ CONNECTION_CHECK_TARGET, /* Check if we have a proper target
connection */
+ CONNECTION_CHECK_RECOVERY /* Check whether server is in recovery
*/
(As noted in GENERAL COMMENT 2 there are still residual references to
pg_is_in_recovery)
Probably this CONNECTION_CHECK_RECOVERY case should be removed.
====
Kind Regards,
Peter Smith
---
Fujitsu Australia
