New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Npgsql 3.2+ unnecessarily uses prepared transactions on Mono #1592

Closed
cookav opened this Issue Jun 6, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@cookav

cookav commented Jun 6, 2017

New transactions implementation (Npgsql 3.2+) uses prepared transactions on Mono when any other resource is enlisted in the same managed transaction. It happens for example when using managed transactions with Npgsql and NHibernate on Mono. On .NET framework it works fine.

I believe the reason is Mono calls Prepare/Commit on enlistment while .NET framework calls SinglePhaseCommit.

Npgsql 3.1 enlists as IPromotableSinglePhaseNotification and works fine both on .NET framework and Mono.

Steps to reproduce

Run the following code on Mono:

using System;
using System.Transactions;
using Npgsql;

namespace NpgsqlMonoTest
{
    class Program
    {
        private const string ConnectionString = "Server=localhost;User ID=npgsql_tests;Password=npgsql_tests;Database=npgsql_tests;Enlist=true";

        static void Main(string[] args)
        {
            var conn = new NpgsqlConnection(ConnectionString);
            using (var scope = new TransactionScope())
            {
                // NHibernate session enlists like this:
                Transaction.Current.EnlistVolatile(new FakeEnlistmentNotification(),
                    EnlistmentOptions.EnlistDuringPrepareRequired);

                conn.Open();

                using (var cmd = new NpgsqlCommand(@"SELECT COUNT(*) FROM data", conn))
                {
                    Console.WriteLine("Count: {0}", cmd.ExecuteScalar());
                }

                scope.Complete();
            }
        }
    }

    class FakeEnlistmentNotification : IEnlistmentNotification
    {
        public void Prepare(PreparingEnlistment preparingEnlistment)
        {
            preparingEnlistment.Prepared();
        }

        public void Commit(Enlistment enlistment)
        {
            enlistment.Done();
        }

        public void Rollback(Enlistment enlistment)
        {
            enlistment.Done();
        }

        public void InDoubt(Enlistment enlistment)
        {
            enlistment.Done();
        }
    }
}

The issue

Npgsql tries to use prepared transactions and throws if they are disabled (the default):

Exception message: Npgsql.PostgresException: 55000: prepared transactions are disabled

Further technical details

Npgsql version: 3.2.3
PostgreSQL version: 9.6
Operating system: Ubuntu 14.04 + Mono 4.4

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Jun 6, 2017

Member

Hmm, this is an interesting one. Before I spend time of this, can you please test on the latest mono (5) to make sure the issue still exists?

Member

roji commented Jun 6, 2017

Hmm, this is an interesting one. Before I spend time of this, can you please test on the latest mono (5) to make sure the issue still exists?

@cookav

This comment has been minimized.

Show comment
Hide comment
@cookav

cookav Jun 7, 2017

Just tested and it's the same. Mono transactions code has not really changed for years: https://github.com/mono/mono/blob/master/mcs/class/System.Transactions/System.Transactions/Transaction.cs

cookav commented Jun 7, 2017

Just tested and it's the same. Mono transactions code has not really changed for years: https://github.com/mono/mono/blob/master/mcs/class/System.Transactions/System.Transactions/Transaction.cs

@fredericDelaporte

This comment has been minimized.

Show comment
Hide comment
@fredericDelaporte

fredericDelaporte Jul 3, 2017

Contributor

Going back to IPromotableSinglePhaseNotification is likely not adequate. This interface is for delegating the transaction responsibility to an external process, such as a local windows service or even a remote service on the database server.

I think Npgsql should instead register itself as a DurableResource, even if it does not yet support recovery. As showcased in #1625 test case, registering as a durable resource allows its single phase commit to still be executed in presence of a volatile resource as NHibernate.

What causes this trouble to appear on Mono with NHibernate, not on .Net Framework, is likely the fact NHibernate currently enlist with option EnlistDuringPrepareRequired. This option is ignored on mono side. When enlisting without that option, the same trouble appears with .Net Framework. (I have written "currently" on purpose: some upcoming changes for NHibernate v5 will cause this to change (if merged) with an option, and otherwise an additional connection will be enlisted, forcing promotion to distributed - at least from Npgsql stand-point: going through prepare phase; Npgsql being currently volatile, MSDTC will not promote to distributed even with 2 Npgsql connection enlisted. This can impact you @cookav, the PR about that is here.)

This option causes the NHibernate resource to be prepared first. Then if only Npgsql remains, since it supports single phase commit, it is executed as single phase. Without that option, Npgsql may get prepared first or simultaneously (when distributed), but since another resource has not yet voted, it cannot go single phase and go through 2PC.

You should test with code similar to the one in #1625 if registering "Npgsql like resource" as a durable resource would allow it to go again through its single phase under Mono.

Contributor

fredericDelaporte commented Jul 3, 2017

Going back to IPromotableSinglePhaseNotification is likely not adequate. This interface is for delegating the transaction responsibility to an external process, such as a local windows service or even a remote service on the database server.

I think Npgsql should instead register itself as a DurableResource, even if it does not yet support recovery. As showcased in #1625 test case, registering as a durable resource allows its single phase commit to still be executed in presence of a volatile resource as NHibernate.

What causes this trouble to appear on Mono with NHibernate, not on .Net Framework, is likely the fact NHibernate currently enlist with option EnlistDuringPrepareRequired. This option is ignored on mono side. When enlisting without that option, the same trouble appears with .Net Framework. (I have written "currently" on purpose: some upcoming changes for NHibernate v5 will cause this to change (if merged) with an option, and otherwise an additional connection will be enlisted, forcing promotion to distributed - at least from Npgsql stand-point: going through prepare phase; Npgsql being currently volatile, MSDTC will not promote to distributed even with 2 Npgsql connection enlisted. This can impact you @cookav, the PR about that is here.)

This option causes the NHibernate resource to be prepared first. Then if only Npgsql remains, since it supports single phase commit, it is executed as single phase. Without that option, Npgsql may get prepared first or simultaneously (when distributed), but since another resource has not yet voted, it cannot go single phase and go through 2PC.

You should test with code similar to the one in #1625 if registering "Npgsql like resource" as a durable resource would allow it to go again through its single phase under Mono.

@cookav

This comment has been minimized.

Show comment
Hide comment
@cookav

cookav Jul 3, 2017

I didn't test it but based on the source code (https://github.com/mono/mono/blob/master/mcs/class/System.Transactions/System.Transactions/Transaction.cs#L378-L414) it should work. Mono should prepare/commit all volatile resources and single-phase commit a single durable resource (durable resources are treated similarly as IPromotableSinglePhaseNotification). Npgsql would only need to register using EnlistDurable (Guid,ISinglePhaseNotification,EnlistmentOptions) with enlistment options set to EnlistmentOptions.None.

cookav commented Jul 3, 2017

I didn't test it but based on the source code (https://github.com/mono/mono/blob/master/mcs/class/System.Transactions/System.Transactions/Transaction.cs#L378-L414) it should work. Mono should prepare/commit all volatile resources and single-phase commit a single durable resource (durable resources are treated similarly as IPromotableSinglePhaseNotification). Npgsql would only need to register using EnlistDurable (Guid,ISinglePhaseNotification,EnlistmentOptions) with enlistment options set to EnlistmentOptions.None.

@roji roji added this to the 3.3 milestone Jul 22, 2017

@roji roji modified the milestones: 4.0, 4.1 Mar 14, 2018

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Jun 9, 2018

Member

I don't really have time to dive into this, but implementing recovery (and therefore enlisting durably) is planned in #1378, and until then I don't think we'll be doing any change in how we enlist...

So am going to close this for now, but can reopen upon request.

Member

roji commented Jun 9, 2018

I don't really have time to dive into this, but implementing recovery (and therefore enlisting durably) is planned in #1378, and until then I don't think we'll be doing any change in how we enlist...

So am going to close this for now, but can reopen upon request.

@roji roji closed this Jun 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment