Skip to content

Commit 2588883

Browse files
committed
test(pgsql): cover an unanswered COMMIT without a usable digest
Case H: with query_digests and commands_stats off, a COMMIT killed in flight (simple, extended, after another statement) disconnects the client; with query_digests_max_query_length shortened, a COMMIT past the end of the digest does too, and a long query without a COMMIT still poisons the session. The check of statements pipelined behind the ROLLBACK waited for ever when the proxy closed the connection; it now stops, and frees the results it reads.
1 parent 26433a9 commit 2588883

1 file changed

Lines changed: 94 additions & 6 deletions

File tree

‎test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp‎

Lines changed: 94 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,9 @@
5454
* The next statement may go to another hostgroup or be pipelined right behind
5555
* the ROLLBACK (over either protocol), and SET values changed in the transaction are
5656
* back afterwards. Run with both protocols.
57+
* * A COMMIT in flight is still found without a usable digest: with query_digests
58+
* and commands_stats off, and in a query longer than query_digests_max_query_length.
59+
* A long query without a COMMIT is not a reason to disconnect.
5760
* * Cases F and G run in four TLS variants per backend protocol: the client talks TLS
5861
* to the proxy or not, the proxy talks TLS to the backend or not. Each variant checks,
5962
* from PostgreSQL's side, that the backend leg is what it was set to.
@@ -118,7 +121,26 @@ static PGconn* open_conn(const char* host, int port,
118121

119122
// Drive the admin interface (PgSQL-protocol admin port, 6132 by default) to
120123
// toggle the preserve_client_on_broken_backend_in_tx boolean.
124+
static bool set_admin_var(const char* var, const char* value);
125+
121126
static bool set_admin_bool(const char* var, bool value) {
127+
return set_admin_var(var, value ? "true" : "false");
128+
}
129+
130+
// The value of a PgSQL admin variable ("" if it cannot be read).
131+
static std::string get_admin_var(const char* var) {
132+
PGconn* admin = open_conn(cl.pgsql_admin_host, cl.pgsql_admin_port,
133+
cl.admin_username, cl.admin_password, "ProxySQL admin (PgSQL protocol)");
134+
if (!admin) return "";
135+
const std::string q = std::string("SELECT variable_value FROM global_variables WHERE variable_name='pgsql-") + var + "'";
136+
PGresult* r = PQexec(admin, q.c_str());
137+
std::string out = (PQresultStatus(r) == PGRES_TUPLES_OK && PQntuples(r) == 1) ? PQgetvalue(r, 0, 0) : "";
138+
PQclear(r);
139+
PQfinish(admin);
140+
return out;
141+
}
142+
143+
static bool set_admin_var(const char* var, const char* value) {
122144
PGconn* admin = open_conn(cl.pgsql_admin_host, cl.pgsql_admin_port,
123145
cl.admin_username, cl.admin_password,
124146
"ProxySQL admin (PgSQL protocol)");
@@ -129,7 +151,7 @@ static bool set_admin_bool(const char* var, bool value) {
129151
char q[256];
130152
snprintf(q, sizeof(q),
131153
"UPDATE global_variables SET variable_value='%s' WHERE variable_name='pgsql-%s'",
132-
value ? "true" : "false", var);
154+
value, var);
133155
PGresult* r = PQexec(admin, q);
134156
bool ok1 = (PQresultStatus(r) == PGRES_COMMAND_OK);
135157
PQclear(r);
@@ -1175,13 +1197,21 @@ static void check_pipelined_behind_rollback(const char* mode) {
11751197
std::string unlocked;
11761198
for (int syncs = 0; sent && syncs < 4;) {
11771199
PGresult* r = PQgetResult(cli);
1178-
if (r == nullptr) continue;
1200+
if (r == nullptr) {
1201+
// The proxy ended the session: PQgetResult then returns NULL for ever.
1202+
if (PQstatus(cli) != CONNECTION_OK) sent = false;
1203+
continue;
1204+
}
11791205
if (PQresultStatus(r) == PGRES_PIPELINE_SYNC) syncs++;
11801206
else if (PQresultStatus(r) == PGRES_TUPLES_OK && PQntuples(r) == 1) unlocked = PQgetvalue(r, 0, 0);
11811207
PQclear(r);
11821208
}
1183-
while (sent && PQgetResult(cli) != nullptr) {}
1184-
PQexitPipelineMode(cli);
1209+
while (sent) {
1210+
PGresult* r = PQgetResult(cli);
1211+
if (r == nullptr) break;
1212+
PQclear(r);
1213+
}
1214+
if (PQstatus(cli) == CONNECTION_OK) PQexitPipelineMode(cli);
11851215
// Only the INSERT after the ROLLBACK is committed; the one inside the old transaction is not.
11861216
if (sent && unlocked == "t" && scalar(cli, "SELECT coalesce(string_agg(x::text, ',' ORDER BY x), 'none') FROM txp_g_rows") == "2") good++;
11871217
}
@@ -1343,6 +1373,62 @@ static void case_G_local_error_keeps_session_state(const char* mode, PGconn* adm
13431373
exec_ok(admin, "LOAD PGSQL SERVERS TO RUNTIME");
13441374
}
13451375

1376+
// --------------------------------------------------------------------------
1377+
// Case H: a COMMIT that may have taken effect is still found when the digest cannot be used: there is
1378+
// none with query_digests and commands_stats off, and the one of a query longer than
1379+
// query_digests_max_query_length is cut. Run once per backend protocol.
1380+
// --------------------------------------------------------------------------
1381+
1382+
static const int CASE_H_CHECKS = 5;
1383+
1384+
static void case_H_digest_unusable(const char* mode) {
1385+
const std::string digests0 = get_admin_var("query_digests");
1386+
const std::string stats0 = get_admin_var("commands_stats");
1387+
const std::string cap0 = get_admin_var("query_digests_max_query_length");
1388+
1389+
const std::string off = std::string(mode) + ", no digest";
1390+
if (set_admin_var("query_digests", "false") && set_admin_var("commands_stats", "false")) {
1391+
check_commit_disconnects(off.c_str(), "COMMIT", [](PGconn* c, const std::string& m) {
1392+
return answer_sqlstate(PQexec(c, ("COMMIT /* " + m + " */").c_str()));
1393+
});
1394+
check_commit_disconnects(off.c_str(), "extended COMMIT", [](PGconn* c, const std::string& m) {
1395+
return answer_sqlstate(PQexecParams(c, ("COMMIT /* " + m + " */").c_str(), 0, nullptr, nullptr, nullptr, nullptr, 0));
1396+
});
1397+
check_commit_disconnects(off.c_str(), "INSERT; COMMIT", [](PGconn* c, const std::string& m) {
1398+
return answer_sqlstate(PQexec(c, ("INSERT INTO txp_commit_plain VALUES ('nodigest'); COMMIT /* " + m + " */").c_str()));
1399+
});
1400+
} else {
1401+
emit_fail_fill(3, "case H: cannot turn the digests off");
1402+
}
1403+
if (!digests0.empty()) set_admin_var("query_digests", digests0.c_str());
1404+
if (!stats0.empty()) set_admin_var("commands_stats", stats0.c_str());
1405+
1406+
const std::string cut = std::string(mode) + ", digest cut";
1407+
if (set_admin_var("query_digests_max_query_length", "64")) {
1408+
const std::string pad(200, 'x');
1409+
// The COMMIT is past the end of the digest.
1410+
check_commit_disconnects(cut.c_str(), "COMMIT past the cut", [&pad](PGconn* c, const std::string& m) {
1411+
return answer_sqlstate(PQexec(c, ("INSERT INTO txp_commit_plain VALUES ('cut'); /* " + pad + " */ COMMIT /* " + m + " */").c_str()));
1412+
});
1413+
// Control: a long query that cannot commit is no reason to disconnect.
1414+
PGconn* cli = open_proxy();
1415+
bool poisoned = false;
1416+
bool killed = false;
1417+
if (cli && exec_ok(cli, "BEGIN")) {
1418+
KillResult kr = run_poisoning_query(cli, ("SELECT pg_sleep(5) /* " + pad + " */").c_str());
1419+
killed = kr.kill_delivered;
1420+
poisoned = is_sqlstate(kr.pgresult, "25P02") && PQstatus(cli) == CONNECTION_OK;
1421+
PQclear(kr.pgresult);
1422+
if (PQstatus(cli) == CONNECTION_OK) exec_ok(cli, "ROLLBACK");
1423+
}
1424+
ok(killed && poisoned, "case H [%s] long query without a COMMIT: still poisoned (25P02, connection kept)", cut.c_str());
1425+
if (cli) PQfinish(cli);
1426+
} else {
1427+
emit_fail_fill(2, "case H: cannot shorten the digest");
1428+
}
1429+
if (!cap0.empty()) set_admin_var("query_digests_max_query_length", cap0.c_str());
1430+
}
1431+
13461432
// Whether the backend leg of a proxy session is TLS, as PostgreSQL itself sees it: "true", "false", or ""
13471433
// if it could not be told.
13481434
static std::string backend_leg_tls() {
@@ -1376,7 +1462,7 @@ static std::string backend_leg_tls() {
13761462
// the proxy talks TLS to the backend or not. Each variant also checks, from PostgreSQL's side, that the
13771463
// backend leg is what it was set to.
13781464
static const int VARIANT_CHECKS = 1 + CASE_F_CHECKS + CASE_G_CHECKS;
1379-
static const int PER_MODE_CHECKS = CASE_E_CHECKS + 4 * VARIANT_CHECKS;
1465+
static const int PER_MODE_CHECKS = CASE_E_CHECKS + CASE_H_CHECKS + 4 * VARIANT_CHECKS;
13801466

13811467
// Re-creates the backend rows with use_ssl set as asked. The pool is emptied first: a pooled connection
13821468
// keeps the TLS setting it was opened with.
@@ -1422,6 +1508,7 @@ static void cases_E_F_G_all_modes() {
14221508
continue;
14231509
}
14241510
case_E_unanswered_commit(mode);
1511+
case_H_digest_unusable(mode);
14251512
for (const bool client_tls : { false, true }) {
14261513
for (const bool backend_tls : { true, false }) {
14271514
const std::string variant = std::string(mode) + (client_tls ? ", client TLS" : ", client plain") +
@@ -1470,7 +1557,8 @@ int main(int /*argc*/, char** /*argv*/) {
14701557
// case E ok() count = CASE_E_CHECKS per backend protocol (libpq, native)
14711558
// case F and G run in 4 TLS variants per backend protocol (libpq, native), each with
14721559
// 1 check of the backend leg + CASE_F_CHECKS + CASE_G_CHECKS: VARIANT_CHECKS
1473-
// PER_MODE_CHECKS = CASE_E_CHECKS + 4 * VARIANT_CHECKS
1560+
// case H: CASE_H_CHECKS per backend protocol
1561+
// PER_MODE_CHECKS = CASE_E_CHECKS + CASE_H_CHECKS + 4 * VARIANT_CHECKS
14741562
// Total = 17 + 2 * PER_MODE_CHECKS
14751563
plan(17 + 2 * PER_MODE_CHECKS);
14761564

0 commit comments

Comments
 (0)