Repository navigation
NpgsqlBatch does not properly set ConnectorPreparedOn property on its BatchCommands #5458
Description
Steps to reproduce
Add following two tests to https://github.com/npgsql/npgsql/blob/main/test/Npgsql.Tests/PrepareTests.cs file and run them.
[Test]
public void Batch_sets_properly_connector_prepared_on_for_its_batch_commands_auto_prepare()
{
using var dataSource = CreateDataSource(csb =>
{
csb.MaxAutoPrepare = 5;
csb.AutoPrepareMinUsages = 1;
});
using var conn = dataSource.OpenConnection();
using (var batch = new NpgsqlBatch(conn) { BatchCommands = { new("SELECT 1"), new("SELECT 2") } })
{
AssertNumPreparedStatements(conn, 2);
foreach (var batchCommand in batch.BatchCommands)
Assert.IsNotNull(((NpgsqlBatchCommand)batchCommand).ConnectorPreparedOn);
}
}
[Test]
public void Batch_sets_properly_connector_prepared_on_for_its_batch_commands()
{
using var conn = OpenConnectionAndUnprepare();
using (var batch = new NpgsqlBatch(conn) { BatchCommands = { new("SELECT 1"), new("SELECT 2") } })
{
batch.Prepare();
AssertNumPreparedStatements(conn, 2);
foreach (var batchCommand in batch.BatchCommands)
Assert.IsNotNull(((NpgsqlBatchCommand)batchCommand).ConnectorPreparedOn);
}
conn.UnprepareAll();
}
The issue
After bumping Npgsql version from 7.0.1 to 7.0.4 we experienced a serious performance degradation in our performance benchmarks. We did an investigation, and the exact culprit seems to be this fix: #4821
It seems that preparation (for a given batch with batch commands) is unnecessary reset because of ConnectorPreparedOn property being incorrectly (almost) always null.
Further technical details
Please note that the issue is still there for the latest versions of npgsql and PostgresSql. Versions used for the tests from above:
Npgsql version: latest from main (8.0.0)
PostgreSQL version: 15.4
Operating system: Windows 10
Reacted by Anders Larsen
Activity
vonzshik commentedon Nov 27, 2023
Hello. I'm not sure whether your first test is correct (since you never execute the batch none of the queries are going to be auto prepared, so
ConnectorPreparedOnwill indeed benull), the second test does show an issue with explicit prepare which I'm going to fix.stojancho-jefremov commentedon Nov 27, 2023
Hello @vonzshik! Regarding my first test, it seems that you are right. After I changed it to this:
the test stopped failing.
Frankly, we don't use the auto prepare, so I reported that one because I thought there is similar issue as with the explicit prepare. I also looked a bit into the code, but it seems that I didn't get it completely right. Anyway. you can consider adding this test if you find it useful. Thanks!
Fix explicit batch preparation reset
Fix explicit batch preparation reset (#5459)
Fix explicit batch preparation reset (#5459)
Fix explicit batch preparation reset (#5459)
Fix explicit batch preparation reset (#5459)
Fix explicit batch preparation reset (npgsql#5459)
Fix explicit batch preparation reset (npgsql#5459)