Skip to content

Commit 4ee720b

Browse files
jBouyoudjBouyoud
committed
Merge remote-tracking branch 'pr-595/lock-grants' into merge-pr-595
* pr-595/lock-grants: Re add set statement_timeout like for pgLockRole Use object-level locks for concurrent grants to improve parallelism Improve test setup Wrap postgresql_grant resource in a lock Revert changes to master Fix for `Error: could not execute revoke query: pq: tuple concurrently updated`
2 parents 4f3ed82 + 167bdcf commit 4ee720b

File tree

10 files changed

+354
-12
lines changed

10 files changed

+354
-12
lines changed

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ This intent of this fork is to maintain a working version of the provider while
88
This repository aims to be up to date with the original one, and also to add some features that are needed for our use cases.
99
We re-integrated the following PRs that were open on the original repository:
1010

11-
- [Use object-level locks for concurrent grants to improve parallelism](https://github.com/cyrilgdn/terraform-provider-postgresql/pull/595)
11+
- [Use object-level locks for concurrent grants to improve parallelism](https://github.com/cyrilgdn/terraform-provider-postgresql/pull/595) ✅
1212
- [Support role configuration parameters](https://github.com/cyrilgdn/terraform-provider-postgresql/pull/305)
1313

1414
---

‎examples/issues/178/dev.tfrc‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
provider_installation {
2+
dev_overrides {
3+
"cyrilgdn/postgresql" = "../../../"
4+
}
5+
direct {}
6+
}

‎examples/issues/178/locals.tf‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
locals {
2+
read_only_users = toset([for i in range(var.user_ro_count) : "user_ro_${i}"])
3+
read_write_users = toset([for i in range(var.user_rw_count) : "user_rw_${i}"])
4+
}

‎examples/issues/178/main.tf‎

Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,191 @@
1+
terraform {
2+
required_providers {
3+
docker = {
4+
source = "kreuzwerker/docker"
5+
version = ">= 3.0.2"
6+
}
7+
postgresql = {
8+
source = "cyrilgdn/postgresql"
9+
version = ">= 1.25"
10+
}
11+
}
12+
}
13+
14+
provider "docker" {
15+
host = var.docker_host
16+
}
17+
18+
resource "docker_image" "postgres" {
19+
name = var.postgres_image
20+
keep_locally = var.keep_image
21+
}
22+
23+
resource "docker_container" "postgres" {
24+
image = docker_image.postgres.image_id
25+
name = "postgres"
26+
wait = true
27+
ports {
28+
internal = var.POSTGRES_PORT
29+
external = var.POSTGRES_PORT
30+
}
31+
env = [
32+
"POSTGRES_PASSWORD=${var.POSTGRES_PASSWORD}"
33+
]
34+
healthcheck {
35+
test = ["CMD-SHELL", "pg_isready"]
36+
interval = "5s"
37+
timeout = "5s"
38+
retries = 5
39+
start_period = "2s"
40+
}
41+
upload {
42+
file = "/docker-entrypoint-initdb.d/mock-tables.sql"
43+
content = <<EOS
44+
CREATE DATABASE "test" OWNER "${var.POSTGRES_DBNAME}";
45+
\connect ${var.POSTGRES_DBNAME}
46+
47+
DO $$
48+
DECLARE
49+
table_count int := ${var.table_count};
50+
BEGIN
51+
FOR count IN 0..table_count LOOP
52+
EXECUTE format('CREATE TABLE table_%s (test int)', count);
53+
END LOOP;
54+
END $$;
55+
EOS
56+
}
57+
}
58+
59+
provider "postgresql" {
60+
scheme = "postgres"
61+
host = var.POSTGRES_HOST
62+
port = docker_container.postgres.ports[0].external
63+
database = var.POSTGRES_PASSWORD
64+
username = var.POSTGRES_PASSWORD
65+
password = var.POSTGRES_PASSWORD
66+
sslmode = "disable"
67+
superuser = false
68+
lock_grants = true
69+
}
70+
71+
resource "postgresql_role" "readonly_role" {
72+
name = "readonly"
73+
login = false
74+
superuser = false
75+
create_database = false
76+
create_role = false
77+
inherit = false
78+
replication = false
79+
connection_limit = -1
80+
}
81+
82+
resource "postgresql_role" "readwrite_role" {
83+
name = "readwrite"
84+
login = false
85+
superuser = false
86+
create_database = false
87+
create_role = false
88+
inherit = false
89+
replication = false
90+
connection_limit = -1
91+
}
92+
93+
resource "postgresql_grant" "readonly_role" {
94+
database = var.POSTGRES_DBNAME
95+
role = postgresql_role.readonly_role.name
96+
object_type = "table"
97+
schema = "public"
98+
privileges = ["SELECT"]
99+
with_grant_option = false
100+
}
101+
102+
resource "postgresql_grant" "readwrite_role" {
103+
database = var.POSTGRES_DBNAME
104+
role = postgresql_role.readwrite_role.name
105+
object_type = "table"
106+
schema = "public"
107+
privileges = ["SELECT", "INSERT", "UPDATE", "DELETE"]
108+
with_grant_option = false
109+
}
110+
111+
resource "postgresql_role" "readonly_users" {
112+
for_each = local.read_only_users
113+
name = each.value
114+
roles = [postgresql_role.readonly_role.name]
115+
login = true
116+
superuser = false
117+
create_database = false
118+
create_role = false
119+
inherit = true
120+
replication = false
121+
connection_limit = -1
122+
}
123+
124+
resource "postgresql_role" "readwrite_users" {
125+
for_each = local.read_write_users
126+
name = each.value
127+
roles = [postgresql_role.readonly_role.name]
128+
login = true
129+
superuser = false
130+
create_database = false
131+
create_role = false
132+
inherit = true
133+
replication = false
134+
connection_limit = -1
135+
}
136+
137+
resource "postgresql_grant" "connect_db_readonly_role" {
138+
for_each = postgresql_role.readonly_users
139+
database = var.POSTGRES_DBNAME
140+
object_type = "database"
141+
privileges = ["CREATE", "CONNECT"]
142+
role = each.value.name
143+
}
144+
145+
resource "postgresql_grant" "connect_db_readwrite_role" {
146+
for_each = postgresql_role.readwrite_users
147+
database = var.POSTGRES_DBNAME
148+
object_type = "database"
149+
privileges = ["CREATE", "CONNECT"]
150+
role = each.value.name
151+
}
152+
153+
resource "postgresql_grant" "usage_readonly_role" {
154+
for_each = postgresql_role.readonly_users
155+
database = var.POSTGRES_DBNAME
156+
role = each.value.name
157+
object_type = "schema"
158+
schema = "public"
159+
privileges = ["USAGE"]
160+
with_grant_option = false
161+
}
162+
163+
resource "postgresql_grant" "usage_readwrite_role" {
164+
for_each = postgresql_role.readwrite_users
165+
database = var.POSTGRES_DBNAME
166+
role = each.value.name
167+
object_type = "schema"
168+
schema = "public"
169+
privileges = ["USAGE"]
170+
with_grant_option = false
171+
}
172+
173+
resource "postgresql_grant" "select_readonly_role" {
174+
for_each = postgresql_role.readonly_users
175+
database = var.POSTGRES_DBNAME
176+
role = each.value.name
177+
object_type = "table"
178+
schema = "public"
179+
privileges = ["SELECT"]
180+
with_grant_option = false
181+
}
182+
183+
resource "postgresql_grant" "crud_readwrite_role" {
184+
for_each = postgresql_role.readwrite_users
185+
database = var.POSTGRES_DBNAME
186+
role = each.value.name
187+
object_type = "table"
188+
schema = "public"
189+
privileges = ["SELECT", "UPDATE", "INSERT", "DELETE"]
190+
with_grant_option = false
191+
}

‎examples/issues/178/test.tftest.hcl‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
run "concurrent_grants" {
2+
command = apply
3+
}

‎examples/issues/178/variables.tf‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
variable "postgres_image" {
2+
description = "Which postgres docker image to use"
3+
default = "postgres:17"
4+
type = string
5+
sensitive = false
6+
}
7+
8+
variable "docker_host" {
9+
description = "Socket path to docker host to use for testing"
10+
default = "unix:///var/run/docker.sock"
11+
type = string
12+
sensitive = false
13+
}
14+
15+
variable "table_count" {
16+
description = "Number of mock tables to create"
17+
default = 300
18+
type = number
19+
sensitive = false
20+
}
21+
22+
variable "user_ro_count" {
23+
description = "Number of mock RO users to create"
24+
default = 30
25+
type = number
26+
sensitive = false
27+
}
28+
29+
variable "user_rw_count" {
30+
description = "Number of mock RW users to create"
31+
default = 30
32+
type = number
33+
sensitive = false
34+
}
35+
36+
variable "POSTGRES_DBNAME" {
37+
default = "postgres"
38+
type = string
39+
sensitive = false
40+
}
41+
42+
variable "POSTGRES_USER" {
43+
default = "postgres"
44+
type = string
45+
sensitive = false
46+
}
47+
48+
variable "POSTGRES_PASSWORD" {
49+
description = "Password for docker POSTGRES_USER"
50+
default = "postgres"
51+
type = string
52+
sensitive = false
53+
}
54+
55+
variable "POSTGRES_HOST" {
56+
default = "127.0.0.1"
57+
type = string
58+
sensitive = false
59+
}
60+
61+
variable "POSTGRES_PORT" {
62+
description = "Which port postgres should listen on."
63+
default = 5432
64+
type = number
65+
sensitive = false
66+
}
67+
68+
variable "keep_image" {
69+
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."
70+
default = true
71+
type = bool
72+
sensitive = false
73+
}

‎postgresql/config.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,10 @@ func (db *DBConnection) isSuperuser() (bool, error) {
159159
return superuser, nil
160160
}
161161

162+
func (db *DBConnection) IsLockGrants() bool {
163+
return db.client.IsLockGrants()
164+
}
165+
162166
type ClientCertificateConfig struct {
163167
CertificatePath string
164168
KeyPath string
@@ -183,6 +187,7 @@ type Config struct {
183187
SSLClientCert *ClientCertificateConfig
184188
SSLRootCertPath string
185189
GCPIAMImpersonateServiceAccount string
190+
LockGrants bool
186191
}
187192

188193
// Client struct holding connection string
@@ -333,6 +338,10 @@ func (c *Client) Connect() (*DBConnection, error) {
333338
return conn, nil
334339
}
335340

341+
func (c *Client) IsLockGrants() bool {
342+
return c.config.LockGrants
343+
}
344+
336345
// fingerprintCapabilities queries PostgreSQL to populate a local catalog of
337346
// capabilities. This is only run once per Client.
338347
func fingerprintCapabilities(db *sql.DB) (*semver.Version, error) {

‎postgresql/helpers.go‎

Lines changed: 53 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -586,14 +586,62 @@ func pgLockRole(txn *sql.Tx, role string) error {
586586
return nil
587587
}
588588

589-
// Lock a database and all his members to avoid concurrent updates on some resources
590-
func pgLockDatabase(txn *sql.Tx, database string) error {
591-
// Disable statement timeout for this connection otherwise the lock could fail
589+
func generateGrantLockID(database, objectType, schema, objectName string) string {
590+
switch objectType {
591+
case "database":
592+
return fmt.Sprintf("grant:db:%s", database)
593+
594+
case "schema":
595+
return fmt.Sprintf("grant:schema:%s.%s", database, schema)
596+
597+
case "foreign_data_wrapper":
598+
return fmt.Sprintf("grant:fdw:%s.%s", database, objectName)
599+
600+
case "foreign_server":
601+
return fmt.Sprintf("grant:srv:%s.%s", database, objectName)
602+
603+
case "table", "sequence", "column":
604+
if objectName == "" {
605+
return fmt.Sprintf("grant:schema:%s.%s", database, schema)
606+
}
607+
return fmt.Sprintf("grant:%s:%s.%s.%s", objectType, database, schema, objectName)
608+
609+
case "function", "procedure", "routine":
610+
if objectName == "" {
611+
return fmt.Sprintf("grant:schema:%s.%s", database, schema)
612+
}
613+
funcName := strings.Split(objectName, "(")[0]
614+
return fmt.Sprintf("grant:%s:%s.%s.%s", objectType, database, schema, funcName)
615+
616+
default:
617+
return fmt.Sprintf("grant:db:%s", database)
618+
}
619+
}
620+
621+
func pgLockGrantTarget(txn *sql.Tx, d *schema.ResourceData) error {
592622
if _, err := txn.Exec("SET statement_timeout = 0"); err != nil {
593623
return fmt.Errorf("could not disable statement_timeout: %w", err)
594624
}
595-
if _, err := txn.Exec("SELECT pg_advisory_xact_lock(oid::bigint) FROM pg_database WHERE datname = $1", database); err != nil {
596-
return fmt.Errorf("could not get advisory lock for database %s: %w", database, err)
625+
626+
database := d.Get("database").(string)
627+
objectType := d.Get("object_type").(string)
628+
schemaName := d.Get("schema").(string)
629+
objects := d.Get("objects").(*schema.Set)
630+
631+
if objects.Len() == 0 || objectType == "database" || objectType == "schema" {
632+
lockID := generateGrantLockID(database, objectType, schemaName, "")
633+
if _, err := txn.Exec("SELECT pg_advisory_xact_lock(hashtext($1)::bigint)", lockID); err != nil {
634+
return fmt.Errorf("could not acquire advisory lock for %s: %w", lockID, err)
635+
}
636+
return nil
637+
}
638+
639+
for _, obj := range objects.List() {
640+
objectName := obj.(string)
641+
lockID := generateGrantLockID(database, objectType, schemaName, objectName)
642+
if _, err := txn.Exec("SELECT pg_advisory_xact_lock(hashtext($1)::bigint)", lockID); err != nil {
643+
return fmt.Errorf("could not acquire advisory lock for %s: %w", lockID, err)
644+
}
597645
}
598646

599647
return nil

0 commit comments

Comments
 (0)