fix: Stop locking catalog for every resources. (#80)

* fix: Stop locking catalog for every resources.


Historically, this provider takes a (golang) lock on a database for every resource of this database.
So only one create/update/delete of resource by database can be done at the same time.

(As it's a RWLock, read of resources can be parallelized)

Since #5 refactoring, I mistakenly change the lock which now takes a write lock even for read operations
(basically the plan part of Terraform). So provider was slower since v1.10

We could fix this and take a read lock for read operations as before but actually I don't see the point of this lock.
For me, most operations could be done at the same time.

Most of the request are now done in a transaction, the only risk is that a transaction will fail to commit
if multiple resources modifies the same database line.
That's the case for `postgresql_default_privileges`, one `pg_default_acl` row could represents multiple resources,
so this PR takes a lock on the owner role to avoid that.

This should make the provider way faster.

Fix #48

Author: cyrilgdn

postgresql/config.go

@@ -141,13 +141,6 @@ type Client struct {
 	config Config
 
 	databaseName string
-
-	// PostgreSQL lock on pg_catalog.  Many of the operations that Terraform
-	// performs are not permitted to be concurrent.  Unlike traditional
-	// PostgreSQL tables that use MVCC, many of the PostgreSQL system
-	// catalogs look like tables, but are not in-fact able to be
-	// concurrently updated.
-	catalogLock sync.RWMutex
 }
 
 // NewClient returns client config for the specified database.

postgresql/helpers.go

@@ -14,9 +14,6 @@ func PGResourceFunc(fn func(*DBConnection, *schema.ResourceData) error) func(*sc
 	return func(d *schema.ResourceData, meta interface{}) error {
 		client := meta.(*Client)
 
-		client.catalogLock.Lock()
-		defer client.catalogLock.Unlock()
-
 		db, err := client.Connect()
 		if err != nil {
 			return err
@@ -30,9 +27,6 @@ func PGResourceExistsFunc(fn func(*DBConnection, *schema.ResourceData) (bool, er
 	return func(d *schema.ResourceData, meta interface{}) (bool, error) {
 		client := meta.(*Client)
 
-		client.catalogLock.Lock()
-		defer client.catalogLock.Unlock()
-
 		db, err := client.Connect()
 		if err != nil {
 			return false, err
@@ -447,3 +441,19 @@ func getRoleOID(db QueryAble, role string) (int, error) {
 	}
 	return oid, nil
 }
+
+// Lock a role and all his members to avoid concurrent updates on some resources
+func pgLockRole(txn *sql.Tx, role string) error {
+	if _, err := txn.Exec("SELECT pg_advisory_xact_lock(oid::bigint) FROM pg_roles WHERE rolname = $1", role); err != nil {
+		return fmt.Errorf("could not get advisory lock for role %s: %w", role, err)
+	}
+
+	if _, err := txn.Exec(
+		"SELECT pg_advisory_xact_lock(member::bigint) FROM pg_auth_members JOIN pg_roles ON roleid = pg_roles.oid WHERE rolname = $1",
+		role,
+	); err != nil {
+		return fmt.Errorf("could not get advisory lock for members of role %s: %w", role, err)
+	}
+
+	return nil
+}

postgresql/resource_postgresql_database.go

@@ -122,6 +122,14 @@ func createDatabase(db *DBConnection, d *schema.ResourceData) error {
 
 	var err error
 	if owner != "" {
+		// Take a lock on db currentUser to avoid multiple database creation at the same time
+		// It can fail if they grant the same owner to current at the same time as it's not done in transaction.
+		lockTxn, err := startTransaction(db.client, "")
+		if err := pgLockRole(lockTxn, currentUser); err != nil {
+			return err
+		}
+		defer deferredRollback(lockTxn)
+
 		// Needed in order to set the owner of the db if the connection user is not a
 		// superuser
 		ownerGranted, err := grantRoleMembership(db, owner, currentUser)
@@ -225,6 +233,12 @@ func resourcePostgreSQLDatabaseDelete(db *DBConnection, d *schema.ResourceData)
 	var dropWithForce string
 	var err error
 	if owner != "" {
+		lockTxn, err := startTransaction(db.client, "")
+		if err := pgLockRole(lockTxn, currentUser); err != nil {
+			return err
+		}
+		defer deferredRollback(lockTxn)
+
 		// Needed in order to set the owner of the db if the connection user is not a
 		// superuser
 		ownerGranted, err := grantRoleMembership(db, owner, currentUser)
@@ -433,6 +447,12 @@ func setDBOwner(db *DBConnection, d *schema.ResourceData) error {
 	}
 	currentUser := db.client.config.getDatabaseUsername()
 
+	lockTxn, err := startTransaction(db.client, "")
+	if err := pgLockRole(lockTxn, currentUser); err != nil {
+		return err
+	}
+	defer deferredRollback(lockTxn)
+
 	//needed in order to set the owner of the db if the connection user is not a superuser
 	ownerGranted, err := grantRoleMembership(db, owner, currentUser)
 	if err != nil {

postgresql/resource_postgresql_default_privileges.go

@@ -114,6 +114,10 @@ func resourcePostgreSQLDefaultPrivilegesCreate(db *DBConnection, d *schema.Resou
 	}
 	defer deferredRollback(txn)
 
+	if err := pgLockRole(txn, owner); err != nil {
+		return err
+	}
+
 	// Needed in order to set the owner of the db if the connection user is not a superuser
 	if err := withRolesGranted(txn, []string{owner}, func() error {
 
@@ -156,6 +160,10 @@ func resourcePostgreSQLDefaultPrivilegesDelete(db *DBConnection, d *schema.Resou
 	}
 	defer deferredRollback(txn)
 
+	if err := pgLockRole(txn, owner); err != nil {
+		return err
+	}
+
 	// Needed in order to set the owner of the db if the connection user is not a superuser
 	if err := withRolesGranted(txn, []string{owner}, func() error {
 		return revokeRoleDefaultPrivileges(txn, d)
@@ -176,6 +184,10 @@ func readRoleDefaultPrivileges(txn *sql.Tx, d *schema.ResourceData) error {
 	pgSchema := d.Get("schema").(string)
 	objectType := d.Get("object_type").(string)
 
+	if err := pgLockRole(txn, owner); err != nil {
+		return err
+	}
+
 	roleOID, err := getRoleOID(txn, role)
 	if err != nil {
 		return err
@@ -208,7 +220,6 @@ func readRoleDefaultPrivileges(txn *sql.Tx, d *schema.ResourceData) error {
 	// and the specified object type (defaclobjtype).
 
 	var privileges pq.ByteaArray
-
 	if err := txn.QueryRow(
 		query, queryArgs...,
 	).Scan(&privileges); err != nil {

postgresql/resource_postgresql_role.go

@@ -311,13 +311,17 @@ func resourcePostgreSQLRoleCreate(db *DBConnection, d *schema.ResourceData) erro
 }
 
 func resourcePostgreSQLRoleDelete(db *DBConnection, d *schema.ResourceData) error {
+	roleName := d.Get(roleNameAttr).(string)
+
 	txn, err := startTransaction(db.client, "")
 	if err != nil {
 		return err
 	}
 	defer deferredRollback(txn)
 
-	roleName := d.Get(roleNameAttr).(string)
+	if err := pgLockRole(txn, roleName); err != nil {
+		return err
+	}
 
 	if !d.Get(roleSkipReassignOwnedAttr).(bool) {
 		if err := withRolesGranted(txn, []string{roleName}, func() error {
@@ -584,6 +588,11 @@ func resourcePostgreSQLRoleUpdate(db *DBConnection, d *schema.ResourceData) erro
 	}
 	defer deferredRollback(txn)
 
+	oldName, _ := d.GetChange(roleNameAttr)
+	if err := pgLockRole(txn, oldName.(string)); err != nil {
+		return err
+	}
+
 	if err := setRoleName(txn, d); err != nil {
 		return err
 	}