Skip to content

Commit bce0e7a

Browse files
authored
fix: mirror prepared statement mode in session pooling (#827)
This might be a two bug layer. First, it's might be an issue with DEALLOCATE sent as a simple query that doesn't get rewritten, which causes a transaction failure and messes up the whole mirroring. But I'm using pgdog in session mode with mirroring to test performance to see if I can switch the database to a cheaper one so I'm not affected by the DEALLOCATE bug on the source. What I noticed is that the mirror traffic gets this error: ``` ERROR: prepared statement "pdo_stmt_00000001" does not exist STATEMENT: DEALLOCATE pdo_stmt_00000001 ``` I also included a fix to speed up the dockerfile build with caches since I iterated on the dockerfile with the fix. I also have a repro with a docker-compose and a simple php app here: https://github.com/costi/pgdog-pdo-repro The flow is like this: 1. In session mode, the client path uses the **effective** prepared-statements setting from: ```rust config.prepared_statements() ``` 2. `ConfigAndUsers::prepared_statements()` disables prepared statements in session mode: ```rust pub fn prepared_statements(&self) -> PreparedStatements { if self.config.general.pooler_mode == PoolerMode::Session { PreparedStatements::Disabled } else { self.config.general.prepared_statements } } ``` 3. The mirror path was not using that effective value. It constructed its prepared-statement state with the raw default/configured level instead. 4. So with: ```toml pooler_mode = "session" prepared_statements = "extended" ``` the source/client path effectively ran with `Disabled`, while the mirror path still ran with `Extended`. 5. That caused the mirror path to rewrite extended-protocol prepared statement names to `__pgdog_*`, while the source path kept the original PDO names like `pdo_stmt_00000001`. 6. Later, PDO sent simple SQL cleanup: ```sql DEALLOCATE pdo_stmt_00000001 ``` 7. The mirror backend only knew the prepared statement under the rewritten `__pgdog_*` name, so PostgreSQL correctly errored: ```text ERROR: prepared statement "pdo_stmt_00000001" does not exist STATEMENT: DEALLOCATE pdo_stmt_00000001 ```
1 parent 7da3e49 commit bce0e7a

8 files changed

Lines changed: 154 additions & 7 deletions

File tree

.github/workflows/ci.yml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ jobs:
117117
sudo -u postgres psql -c 'ALTER SYSTEM SET wal_level TO logical;'
118118
sudo service postgresql restart
119119
bash integration/setup.sh
120-
sudo apt update && sudo apt install -y python3-virtualenv mold
120+
sudo apt update && sudo apt install -y python3-virtualenv mold php-cli php-pgsql
121121
sudo gem install bundler
122122
sudo apt remove -y cmake
123123
sudo pip3 install cmake==3.31.6
@@ -157,6 +157,8 @@ jobs:
157157
run: bash integration/ruby/run.sh
158158
- name: Java
159159
run: bash integration/java/run.sh
160+
- name: Mirror
161+
run: bash integration/mirror/run.sh
160162
- name: SQL
161163
run: bash integration/sql/run.sh
162164
- name: Toxi

integration/mirror/dev.sh

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,15 @@
11
#!/bin/bash
2+
set -euo pipefail
3+
24
SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
5+
36
pushd ${SCRIPT_DIR}/ruby
4-
bundle exec *_spec.rb
7+
export GEM_HOME=~/.gem
8+
mkdir -p ${GEM_HOME}
9+
bundle install
10+
bundle exec rspec *_spec.rb
11+
popd
12+
13+
pushd ${SCRIPT_DIR}/php
14+
bash run.sh
515
popd
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
function connectDb(string $dbName): PDO
6+
{
7+
$pdo = new PDO(
8+
"pgsql:host=127.0.0.1;port=6432;dbname={$dbName}",
9+
"pgdog",
10+
"pgdog",
11+
[
12+
PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
13+
PDO::ATTR_EMULATE_PREPARES => false,
14+
]
15+
);
16+
17+
return $pdo;
18+
}
19+
20+
function sleepForMirror(): void
21+
{
22+
// Mirror delivery is asynchronous; a short delay keeps the test stable.
23+
usleep(500_000);
24+
}
25+
26+
$source = connectDb("pgdog");
27+
$mirror = connectDb("pgdog_mirror");
28+
29+
$source->exec("DROP TABLE IF EXISTS public.mirror_php_test");
30+
$source->exec("CREATE TABLE public.mirror_php_test (id BIGINT PRIMARY KEY, value TEXT NOT NULL)");
31+
sleepForMirror();
32+
33+
$source->beginTransaction();
34+
35+
$insert = $source->prepare("INSERT INTO public.mirror_php_test (id, value) VALUES (?, ?)");
36+
$insert->execute([1, "one"]);
37+
$insert->execute([2, "two"]);
38+
39+
$source->commit();
40+
41+
sleepForMirror();
42+
43+
$sourceCount = (int) $source->query("SELECT COUNT(*) FROM public.mirror_php_test")->fetchColumn();
44+
$mirrorCount = (int) $mirror->query("SELECT COUNT(*) FROM public.mirror_php_test")->fetchColumn();
45+
46+
if ($sourceCount !== 2) {
47+
fwrite(STDERR, "expected 2 rows on source, got {$sourceCount}\n");
48+
exit(1);
49+
}
50+
51+
if ($mirrorCount !== 2) {
52+
fwrite(STDERR, "expected 2 rows on mirror, got {$mirrorCount}\n");
53+
exit(1);
54+
}
55+
56+
$mirrorRows = $mirror
57+
->query("SELECT id, value FROM public.mirror_php_test ORDER BY id")
58+
->fetchAll(PDO::FETCH_ASSOC);
59+
60+
$mirrorRows = array_map(
61+
static fn(array $row): array => [
62+
"id" => (int) $row["id"],
63+
"value" => (string) $row["value"],
64+
],
65+
$mirrorRows
66+
);
67+
68+
$expected = [
69+
["id" => 1, "value" => "one"],
70+
["id" => 2, "value" => "two"],
71+
];
72+
73+
if ($mirrorRows !== $expected) {
74+
fwrite(STDERR, "unexpected mirror rows: " . json_encode($mirrorRows) . "\n");
75+
exit(1);
76+
}
77+
78+
$source->exec("DROP TABLE IF EXISTS public.mirror_php_test");
79+
sleepForMirror();
80+
81+
echo "php mirror test passed\n";

integration/mirror/php/run.sh

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
#!/bin/bash
2+
set -euo pipefail
3+
4+
SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
5+
6+
php "${SCRIPT_DIR}/pdo_mirror.php"

integration/mirror/ruby/Gemfile.lock

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ GEM
5151
PLATFORMS
5252
arm64-darwin-24
5353
ruby
54+
x86_64-linux
5455

5556
DEPENDENCIES
5657
csv

pgdog/src/backend/pool/connection/mirror/handler.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
66
use super::*;
77
use crate::backend::pool::MirrorStats;
8+
use crate::frontend::ClientRequest;
89
use parking_lot::Mutex;
910
use std::sync::Arc;
1011

pgdog/src/backend/pool/connection/mirror/mod.rs

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,6 @@ use crate::frontend::client::TransactionType;
1616
use crate::frontend::{ClientComms, PreparedStatements};
1717
use crate::net::{BackendKeyData, Parameter, Parameters, Stream};
1818

19-
use crate::frontend::ClientRequest;
20-
2119
use super::Error;
2220

2321
pub mod buffer_with_delay;
@@ -51,9 +49,12 @@ pub struct Mirror {
5149

5250
impl Mirror {
5351
fn new(params: &Parameters, config: &ConfigAndUsers) -> Self {
52+
let mut prepared_statements = PreparedStatements::new();
53+
prepared_statements.set_level(config.prepared_statements());
54+
5455
Self {
5556
id: BackendKeyData::new(),
56-
prepared_statements: PreparedStatements::new(),
57+
prepared_statements,
5758
params: params.clone(),
5859
timeouts: Timeouts::from_config(&config.config.general),
5960
stream: Stream::dev_null(),
@@ -173,7 +174,11 @@ impl Mirror {
173174

174175
#[cfg(test)]
175176
mod test {
176-
use crate::{backend::pool::Request, config, net::Query};
177+
use crate::{
178+
backend::pool::Request,
179+
config::{self, PoolerMode, PreparedStatements as PreparedStatementsLevel},
180+
net::{Parameter, Parameters, Query},
181+
};
177182

178183
use super::*;
179184

@@ -319,4 +324,40 @@ mod test {
319324

320325
cluster.shutdown();
321326
}
327+
328+
#[tokio::test]
329+
async fn test_mirror_uses_effective_prepared_statements_level() {
330+
config::load_test();
331+
let mut test_config = (*config::config()).clone();
332+
test_config.config.general.pooler_mode = PoolerMode::Session;
333+
test_config.config.general.prepared_statements = PreparedStatementsLevel::Extended;
334+
let test_config = config::set(test_config).unwrap();
335+
let config = std::sync::Arc::new(test_config);
336+
337+
assert_eq!(
338+
config.config.general.prepared_statements,
339+
PreparedStatementsLevel::Extended
340+
);
341+
assert_eq!(
342+
config.prepared_statements(),
343+
PreparedStatementsLevel::Disabled
344+
);
345+
346+
let params = Parameters::from(vec![
347+
Parameter {
348+
name: "user".into(),
349+
value: "pgdog".into(),
350+
},
351+
Parameter {
352+
name: "database".into(),
353+
value: "pgdog".into(),
354+
},
355+
]);
356+
357+
let mirror = Mirror::new(&params, &config);
358+
assert_eq!(
359+
mirror.prepared_statements.level(),
360+
PreparedStatementsLevel::Disabled
361+
);
362+
}
322363
}

pgdog/src/frontend/prepared_statements/mod.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,12 @@ impl PreparedStatements {
122122
self.local.len()
123123
}
124124

125+
/// Current prepared statements compatibility level.
126+
#[cfg(test)]
127+
pub fn level(&self) -> PreparedStatementsLevel {
128+
self.level
129+
}
130+
125131
/// Is the local cache empty?
126132
pub fn is_empty(&self) -> bool {
127133
self.len_local() == 0
@@ -157,7 +163,6 @@ impl PreparedStatements {
157163
}
158164

159165
/// Set the prepared statements level.
160-
#[cfg(test)]
161166
pub fn set_level(&mut self, level: PreparedStatementsLevel) {
162167
self.level = level;
163168
}

0 commit comments

Comments
 (0)