Execution strategy fixes and sync

Includes test changes done in dotnet/efcore#18691

Tests now run using retrying strategy, hopefully working around the
transient exceptions we're seeing on CI since the migration tests.

Closes #1244

src/EFCore.PG/NpgsqlRetryingExecutionStrategy.cs

@@ -9,7 +9,7 @@ namespace Npgsql.EntityFrameworkCore.PostgreSQL
 {
     public class NpgsqlRetryingExecutionStrategy : ExecutionStrategy
     {
-        private readonly ICollection<string> _additionalErrorCodes;
+        readonly ICollection<string> _additionalErrorCodes;
 
         /// <summary>
         ///     Creates a new instance of <see cref="NpgsqlRetryingExecutionStrategy" />.
@@ -73,9 +73,7 @@ public class NpgsqlRetryingExecutionStrategy : ExecutionStrategy
             : base(context,
                 maxRetryCount,
                 maxRetryDelay)
-        {
-            _additionalErrorCodes = errorCodesToAdd;
-        }
+            =>  _additionalErrorCodes = errorCodesToAdd;
 
         /// <summary>
         ///     Creates a new instance of <see cref="NpgsqlRetryingExecutionStrategy" />.
@@ -90,26 +88,14 @@ public class NpgsqlRetryingExecutionStrategy : ExecutionStrategy
             TimeSpan maxRetryDelay,
             [CanBeNull] ICollection<string> errorCodesToAdd)
             : base(dependencies, maxRetryCount, maxRetryDelay)
-        {
-            _additionalErrorCodes = errorCodesToAdd;
-        }
+            => _additionalErrorCodes = errorCodesToAdd;
 
+        // TODO: Unlike SqlException, which seems to also wrap various transport/IO errors
+        // and expose them via error codes, we have NpgsqlException with an inner exception.
+        // Would be good to provide a way to add these into the additional list.
         protected override bool ShouldRetryOn(Exception exception)
-        {
-            if (_additionalErrorCodes != null)
-            {
-                // TODO: Unlike SqlException, which seems to also wrap various transport/IO errors
-                // and expose them via error codes, we have NpgsqlException with an inner exception.
-                // Would be good to provide a way to add these into the additional list.
-                var postgresException = exception as PostgresException;
-                if (postgresException != null)
-                {
-                    if (_additionalErrorCodes.Contains(postgresException.SqlState))
-                        return true;
-                }
-            }
-
-            return NpgsqlTransientExceptionDetector.ShouldRetryOn(exception);
-        }
+            => exception is PostgresException postgresException &&
+               _additionalErrorCodes?.Contains(postgresException.SqlState) == true
+               || NpgsqlTransientExceptionDetector.ShouldRetryOn(exception);
     }
 }

src/EFCore.PG/Query/Internal/NpgsqlCompiledQueryCacheKeyGenerator.cs

@@ -34,8 +34,8 @@ struct NpgsqlCompiledQueryCacheKey
 
             public override bool Equals(object obj)
                 => !(obj is null)
-                   && obj is NpgsqlCompiledQueryCacheKey
-                   && Equals((NpgsqlCompiledQueryCacheKey)obj);
+                   && obj is NpgsqlCompiledQueryCacheKey key
+                   && Equals(key);
 
             bool Equals(NpgsqlCompiledQueryCacheKey other)
                 => _relationalCompiledQueryCacheKey.Equals(other._relationalCompiledQueryCacheKey)

src/EFCore.PG/Storage/Internal/NpgsqlExecutionStrategy.cs

@@ -9,12 +9,10 @@ namespace Npgsql.EntityFrameworkCore.PostgreSQL.Storage.Internal
 {
     public class NpgsqlExecutionStrategy : IExecutionStrategy
     {
-        private ExecutionStrategyDependencies Dependencies { get; }
+        ExecutionStrategyDependencies Dependencies { get; }
 
         public NpgsqlExecutionStrategy([NotNull] ExecutionStrategyDependencies dependencies)
-        {
-            Dependencies = dependencies;
-        }
+            => Dependencies = dependencies;
 
         public virtual bool RetriesOnFailure => false;
 
@@ -27,14 +25,9 @@ public NpgsqlExecutionStrategy([NotNull] ExecutionStrategyDependencies dependenc
             {
                 return operation(Dependencies.CurrentContext.Context, state);
             }
-            catch (Exception ex)
+            catch (Exception ex) when (ExecutionStrategy.CallOnWrappedException(ex, NpgsqlTransientExceptionDetector.ShouldRetryOn))
             {
-                if (ExecutionStrategy.CallOnWrappedException(ex, NpgsqlTransientExceptionDetector.ShouldRetryOn))
-                {
-                    throw new InvalidOperationException("An exception has been raised that is likely due to a transient failure.", ex);
-                }
-
-                throw;
+                throw new InvalidOperationException("An exception has been raised that is likely due to a transient failure.", ex);
             }
         }
 
@@ -48,14 +41,9 @@ public NpgsqlExecutionStrategy([NotNull] ExecutionStrategyDependencies dependenc
             {
                 return await operation(Dependencies.CurrentContext.Context, state, cancellationToken);
             }
-            catch (Exception ex)
+            catch (Exception ex) when (ExecutionStrategy.CallOnWrappedException(ex, NpgsqlTransientExceptionDetector.ShouldRetryOn))
             {
-                if (ExecutionStrategy.CallOnWrappedException(ex, NpgsqlTransientExceptionDetector.ShouldRetryOn))
-                {
-                    throw new InvalidOperationException("An exception has been raised that is likely due to a transient failure.", ex);
-                }
-
-                throw;
+                throw new InvalidOperationException("An exception has been raised that is likely due to a transient failure.", ex);
             }
         }
     }

test/EFCore.PG.FunctionalTests/CommandInterceptionNpgsqlTest.cs

@@ -4,6 +4,8 @@
 using Microsoft.EntityFrameworkCore.Diagnostics;
 using Microsoft.EntityFrameworkCore.TestUtilities;
 using Microsoft.Extensions.DependencyInjection;
+using Npgsql.EntityFrameworkCore.PostgreSQL.Infrastructure;
+using Npgsql.EntityFrameworkCore.PostgreSQL.Storage.Internal;
 using Npgsql.EntityFrameworkCore.PostgreSQL.TestUtilities;
 using Xunit;
 
@@ -66,6 +68,13 @@ public CommandInterceptionNpgsqlTest(InterceptionNpgsqlFixture fixture)
             public class InterceptionNpgsqlFixture : InterceptionNpgsqlFixtureBase
             {
                 protected override bool ShouldSubscribeToDiagnosticListener => false;
+
+                public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder)
+                {
+                    new NpgsqlDbContextOptionsBuilder(base.AddOptions(builder))
+                        .ExecutionStrategy(d => new NpgsqlExecutionStrategy(d));
+                    return builder;
+                }
             }
         }
 
@@ -80,6 +89,13 @@ public CommandInterceptionWithDiagnosticsNpgsqlTest(InterceptionNpgsqlFixture fi
             public class InterceptionNpgsqlFixture : InterceptionNpgsqlFixtureBase
             {
                 protected override bool ShouldSubscribeToDiagnosticListener => true;
+
+                public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder)
+                {
+                    new NpgsqlDbContextOptionsBuilder(base.AddOptions(builder))
+                        .ExecutionStrategy(d => new NpgsqlExecutionStrategy(d));
+                    return builder;
+                }
             }
         }
     }