From 400dc3215088a3063898b66084184bc1a9f2ab5e Mon Sep 17 00:00:00 2001 From: Kyle Johnson Date: Wed, 20 Sep 2023 14:52:01 -0400 Subject: [PATCH 1/4] Fix for `Error: could not execute revoke query: pq: tuple concurrently updated` --- examples/issues/178/locals.tf | 4 + examples/issues/178/main.tf | 160 ++++++++++++++++++++++++ examples/issues/178/test.sh | 10 ++ examples/issues/178/variables.tf | 39 ++++++ postgresql/helpers.go | 9 ++ postgresql/resource_postgresql_grant.go | 7 ++ 6 files changed, 229 insertions(+) create mode 100644 examples/issues/178/locals.tf create mode 100644 examples/issues/178/main.tf create mode 100755 examples/issues/178/test.sh create mode 100644 examples/issues/178/variables.tf diff --git a/examples/issues/178/locals.tf b/examples/issues/178/locals.tf new file mode 100644 index 00000000..21c56fbc --- /dev/null +++ b/examples/issues/178/locals.tf @@ -0,0 +1,4 @@ +locals { + read_only_users = ["fu", "bar"] + read_write_users = ["blah", "bzz"] +} diff --git a/examples/issues/178/main.tf b/examples/issues/178/main.tf new file mode 100644 index 00000000..8bf86d86 --- /dev/null +++ b/examples/issues/178/main.tf @@ -0,0 +1,160 @@ +terraform { + required_providers { + docker = { + source = "kreuzwerker/docker" + version = ">= 3.0.2" + } + postgresql = { + source = "cyrilgdn/postgresql" + version = "1.21" + } + # postgresql = { + # source = "terraform-fu.bar/terraform-provider-postgresql/postgresql" + # version = ">= 1.20" + # } + } +} + +provider "docker" { + host = "unix:///var/run/docker.sock" +} + +resource "docker_image" "postgres" { + name = var.postgres_image + keep_locally = var.keep_image +} + +resource "docker_container" "postgres" { + image = docker_image.postgres.image_id + name = "postgres" + wait = true + ports { + internal = var.POSTGRES_PORT + external = var.POSTGRES_PORT + } + env = [ + "POSTGRES_PASSWORD=${var.POSTGRES_PASSWORD}" + ] + healthcheck { + test = ["CMD-SHELL", "pg_isready"] + interval = "5s" + timeout = "5s" + retries = 5 + start_period = "2s" + } +} + +provider "postgresql" { + scheme = "postgres" + host = var.POSTGRES_HOST + port = docker_container.postgres.ports[0].external + database = var.POSTGRES_PASSWORD + username = var.POSTGRES_PASSWORD + password = var.POSTGRES_PASSWORD + sslmode = "disable" + superuser = false +} + +resource "postgresql_database" "this" { + name = "test" + owner = var.POSTGRES_USER +} + +resource "postgresql_role" "readonly_role" { + name = "readonly" + login = false + superuser = false + create_database = false + create_role = false + inherit = false + replication = false + connection_limit = -1 +} + +resource "postgresql_role" "readwrite_role" { + name = "readwrite" + login = false + superuser = false + create_database = false + create_role = false + inherit = false + replication = false + connection_limit = -1 +} + +resource "postgresql_grant" "readonly_role" { + database = postgresql_database.this.name + role = postgresql_role.readonly_role.name + object_type = "table" + schema = "public" + privileges = ["SELECT"] + with_grant_option = false +} + +resource "postgresql_grant" "readwrite_role" { + database = postgresql_database.this.name + role = postgresql_role.readwrite_role.name + object_type = "table" + schema = "public" + privileges = ["SELECT", "INSERT", "UPDATE", "DELETE"] + with_grant_option = false +} + +resource "postgresql_role" "readonly_users" { + for_each = toset(local.read_only_users) + name = each.key + roles = [postgresql_role.readonly_role.name] + login = true + superuser = false + create_database = false + create_role = false + inherit = true + replication = false + connection_limit = -1 +} + +resource "postgresql_role" "readwrite_users" { + for_each = toset(local.read_write_users) + name = each.key + roles = [postgresql_role.readonly_role.name] + login = true + superuser = false + create_database = false + create_role = false + inherit = true + replication = false + connection_limit = -1 +} + +resource "postgresql_grant" "connect_db_readonly_role" { + database = postgresql_database.this.name + object_type = "database" + privileges = ["CREATE", "CONNECT"] + role = postgresql_role.readonly_role.name +} + +resource "postgresql_grant" "connect_db_readwrite_role" { + database = postgresql_database.this.name + object_type = "database" + privileges = ["CREATE", "CONNECT"] + role = postgresql_role.readwrite_role.name +} + +resource "postgresql_grant" "usage_readonly_role" { + database = postgresql_database.this.name + role = postgresql_role.readonly_role.name + object_type = "schema" + schema = "public" + privileges = ["USAGE"] + with_grant_option = false +} + +resource "postgresql_grant" "usage_readwrite_role" { + database = postgresql_database.this.name + role = postgresql_role.readwrite_role.name + object_type = "schema" + schema = "public" + privileges = ["USAGE"] + with_grant_option = false +} + diff --git a/examples/issues/178/test.sh b/examples/issues/178/test.sh new file mode 100755 index 00000000..0d341965 --- /dev/null +++ b/examples/issues/178/test.sh @@ -0,0 +1,10 @@ +#!/bin/bash + +set -eou pipefail +i=0 + +while [ $i -lt 10 ]; do + terraform apply -auto-approve + terraform destroy -auto-approve + i=$((i+1)) +done \ No newline at end of file diff --git a/examples/issues/178/variables.tf b/examples/issues/178/variables.tf new file mode 100644 index 00000000..b97a63fe --- /dev/null +++ b/examples/issues/178/variables.tf @@ -0,0 +1,39 @@ +variable "postgres_image" { + description = "Which postgres docker image to use." + default = "postgres:15" + type = string + sensitive = false +} + +variable "POSTGRES_USER" { + default = "postgres" + type = string + sensitive = false +} + +variable "POSTGRES_PASSWORD" { + description = "Password for docker POSTGRES_USER" + default = "postgres" + type = string + sensitive = false +} + +variable "POSTGRES_HOST" { + default = "127.0.0.1" + type = string + sensitive = false +} + +variable "POSTGRES_PORT" { + description = "Which port postgres should listen on." + default = 5432 + type = number + sensitive = false +} + +variable "keep_image" { + description = "If true, then the Docker image won't be deleted on destroy operation. If this is false, it will delete the image from the docker local storage on destroy operation." + default = true + type = bool + sensitive = false +} diff --git a/postgresql/helpers.go b/postgresql/helpers.go index 54e462d9..11cd756a 100644 --- a/postgresql/helpers.go +++ b/postgresql/helpers.go @@ -586,6 +586,15 @@ func pgLockRole(txn *sql.Tx, role string) error { return nil } +// Lock a schema avoid concurrent updates during revoke query. +func pgLockSchema(txn *sql.Tx, schema string) error { + if _, err := txn.Exec("SELECT pg_advisory_xact_lock(oid::bigint) FROM pg_catalog.pg_namespace WHERE nspname = $1", schema); err != nil { + return fmt.Errorf("could not get advisory lock for schema %s: %w", schema, err) + } + + return nil +} + // Lock a database and all his members to avoid concurrent updates on some resources func pgLockDatabase(txn *sql.Tx, database string) error { // Disable statement timeout for this connection otherwise the lock could fail diff --git a/postgresql/resource_postgresql_grant.go b/postgresql/resource_postgresql_grant.go index fb93c479..36227899 100644 --- a/postgresql/resource_postgresql_grant.go +++ b/postgresql/resource_postgresql_grant.go @@ -723,6 +723,13 @@ func revokeRolePrivileges(txn *sql.Tx, d *schema.ResourceData, usePrevious bool) // Query is empty, don't run anything return nil } + + // Obtain a lock to avoid `Error: could not execute revoke query: pq: tuple concurrently updated` + schema := d.Get("schema").(string) + if err := pgLockSchema(txn, schema); err != nil { + return err + } + if _, err := txn.Exec(query); err != nil { return fmt.Errorf("could not execute revoke query: %w", err) } From 9bd3fee994d64a276504d959dfdcbe1509ace1ce Mon Sep 17 00:00:00 2001 From: Gregory Eric Sanderson Date: Thu, 13 Mar 2025 14:13:34 -0400 Subject: [PATCH 2/4] Revert changes to master --- postgresql/helpers.go | 9 --------- postgresql/resource_postgresql_grant.go | 7 ------- 2 files changed, 16 deletions(-) diff --git a/postgresql/helpers.go b/postgresql/helpers.go index 11cd756a..54e462d9 100644 --- a/postgresql/helpers.go +++ b/postgresql/helpers.go @@ -586,15 +586,6 @@ func pgLockRole(txn *sql.Tx, role string) error { return nil } -// Lock a schema avoid concurrent updates during revoke query. -func pgLockSchema(txn *sql.Tx, schema string) error { - if _, err := txn.Exec("SELECT pg_advisory_xact_lock(oid::bigint) FROM pg_catalog.pg_namespace WHERE nspname = $1", schema); err != nil { - return fmt.Errorf("could not get advisory lock for schema %s: %w", schema, err) - } - - return nil -} - // Lock a database and all his members to avoid concurrent updates on some resources func pgLockDatabase(txn *sql.Tx, database string) error { // Disable statement timeout for this connection otherwise the lock could fail diff --git a/postgresql/resource_postgresql_grant.go b/postgresql/resource_postgresql_grant.go index 36227899..fb93c479 100644 --- a/postgresql/resource_postgresql_grant.go +++ b/postgresql/resource_postgresql_grant.go @@ -723,13 +723,6 @@ func revokeRolePrivileges(txn *sql.Tx, d *schema.ResourceData, usePrevious bool) // Query is empty, don't run anything return nil } - - // Obtain a lock to avoid `Error: could not execute revoke query: pq: tuple concurrently updated` - schema := d.Get("schema").(string) - if err := pgLockSchema(txn, schema); err != nil { - return err - } - if _, err := txn.Exec(query); err != nil { return fmt.Errorf("could not execute revoke query: %w", err) } From cc87fd0bd4de05daf445cdda69e334b63d8daec0 Mon Sep 17 00:00:00 2001 From: Gregory Eric Sanderson Date: Thu, 13 Mar 2025 14:13:44 -0400 Subject: [PATCH 3/4] Wrap postgresql_grant resource in a lock --- postgresql/config.go | 9 +++++++++ postgresql/provider.go | 11 ++++++++++- postgresql/resource_postgresql_grant.go | 5 +++-- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/postgresql/config.go b/postgresql/config.go index c2f1410c..013dcfe9 100644 --- a/postgresql/config.go +++ b/postgresql/config.go @@ -159,6 +159,10 @@ func (db *DBConnection) isSuperuser() (bool, error) { return superuser, nil } +func (db *DBConnection) IsLockGrants() bool { + return db.client.IsLockGrants() +} + type ClientCertificateConfig struct { CertificatePath string KeyPath string @@ -183,6 +187,7 @@ type Config struct { SSLClientCert *ClientCertificateConfig SSLRootCertPath string GCPIAMImpersonateServiceAccount string + LockGrants bool } // Client struct holding connection string @@ -333,6 +338,10 @@ func (c *Client) Connect() (*DBConnection, error) { return conn, nil } +func (c *Client) IsLockGrants() bool { + return c.config.LockGrants +} + // fingerprintCapabilities queries PostgreSQL to populate a local catalog of // capabilities. This is only run once per Client. func fingerprintCapabilities(db *sql.DB) (*semver.Version, error) { diff --git a/postgresql/provider.go b/postgresql/provider.go index 8bc7546d..680698f5 100644 --- a/postgresql/provider.go +++ b/postgresql/provider.go @@ -3,9 +3,10 @@ package postgresql import ( "context" "fmt" + "os" + "github.com/aws/aws-sdk-go-v2/credentials" "github.com/aws/aws-sdk-go-v2/service/sts" - "os" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" @@ -23,6 +24,7 @@ import ( const ( defaultProviderMaxOpenConnections = 20 defaultExpectedPostgreSQLVersion = "9.0.0" + defaultLockGrants = false ) // Provider returns a terraform.ResourceProvider. @@ -199,6 +201,12 @@ func Provider() *schema.Provider { Description: "Specify the expected version of PostgreSQL.", ValidateFunc: validateExpectedVersion, }, + "lock_grants": { + Type: schema.TypeBool, + Optional: true, + Default: defaultLockGrants, + Description: "Wrap GRANT/REVOKEs in a lock to avoid executing them in parallel.", + }, }, ResourcesMap: map[string]*schema.Resource{ @@ -382,6 +390,7 @@ func providerConfigure(d *schema.ResourceData) (interface{}, error) { ExpectedVersion: version, SSLRootCertPath: d.Get("sslrootcert").(string), GCPIAMImpersonateServiceAccount: d.Get("gcp_iam_impersonate_service_account").(string), + LockGrants: d.Get("lock_grants").(bool), } if value, ok := d.GetOk("clientcert"); ok { diff --git a/postgresql/resource_postgresql_grant.go b/postgresql/resource_postgresql_grant.go index fb93c479..80fd5fda 100644 --- a/postgresql/resource_postgresql_grant.go +++ b/postgresql/resource_postgresql_grant.go @@ -180,7 +180,7 @@ func resourcePostgreSQLGrantCreateOrUpdate(db *DBConnection, d *schema.ResourceD return err } - if objectType == "database" { + if db.IsLockGrants() || objectType == "database" { if err := pgLockDatabase(txn, database); err != nil { return err } @@ -238,7 +238,8 @@ func resourcePostgreSQLGrantDelete(db *DBConnection, d *schema.ResourceData) err } objectType := d.Get("object_type").(string) - if objectType == "database" { + + if db.IsLockGrants() || objectType == "database" { if err := pgLockDatabase(txn, database); err != nil { return err } From 796c9ec8b17abd7d986a6bb6d75ff6174077efb0 Mon Sep 17 00:00:00 2001 From: Gregory Eric Sanderson Date: Thu, 13 Mar 2025 14:13:48 -0400 Subject: [PATCH 4/4] Improve test setup --- examples/issues/178/dev.tfrc | 6 ++ examples/issues/178/locals.tf | 4 +- examples/issues/178/main.tf | 97 +++++++++++++++++++---------- examples/issues/178/test.sh | 10 --- examples/issues/178/test.tftest.hcl | 3 + examples/issues/178/variables.tf | 38 ++++++++++- 6 files changed, 111 insertions(+), 47 deletions(-) create mode 100644 examples/issues/178/dev.tfrc delete mode 100755 examples/issues/178/test.sh create mode 100644 examples/issues/178/test.tftest.hcl diff --git a/examples/issues/178/dev.tfrc b/examples/issues/178/dev.tfrc new file mode 100644 index 00000000..53cb5de4 --- /dev/null +++ b/examples/issues/178/dev.tfrc @@ -0,0 +1,6 @@ +provider_installation { + dev_overrides { + "cyrilgdn/postgresql" = "../../../" + } + direct {} +} diff --git a/examples/issues/178/locals.tf b/examples/issues/178/locals.tf index 21c56fbc..1c78652e 100644 --- a/examples/issues/178/locals.tf +++ b/examples/issues/178/locals.tf @@ -1,4 +1,4 @@ locals { - read_only_users = ["fu", "bar"] - read_write_users = ["blah", "bzz"] + read_only_users = toset([for i in range(var.user_ro_count) : "user_ro_${i}"]) + read_write_users = toset([for i in range(var.user_rw_count) : "user_rw_${i}"]) } diff --git a/examples/issues/178/main.tf b/examples/issues/178/main.tf index 8bf86d86..4b2db2c7 100644 --- a/examples/issues/178/main.tf +++ b/examples/issues/178/main.tf @@ -6,17 +6,13 @@ terraform { } postgresql = { source = "cyrilgdn/postgresql" - version = "1.21" + version = ">= 1.25" } - # postgresql = { - # source = "terraform-fu.bar/terraform-provider-postgresql/postgresql" - # version = ">= 1.20" - # } } } provider "docker" { - host = "unix:///var/run/docker.sock" + host = var.docker_host } resource "docker_image" "postgres" { @@ -42,22 +38,34 @@ resource "docker_container" "postgres" { retries = 5 start_period = "2s" } + upload { + file = "/docker-entrypoint-initdb.d/mock-tables.sql" + content = <