Skip to content

Conversation

@pyramation
Copy link
Contributor

Add PostgreSQL role and grant management specification

Summary

Adds a SPEC.md document that specifies patterns and requirements for creating, granting, and managing PostgreSQL roles and users. This specification addresses race conditions, idempotency, and SQL safety concerns identified in the current implementation (related to PR #325).

The document covers:

  • Three required patterns: CREATE ROLE with concurrency protection, GRANT with concurrency protection, and safe REVOKE/DROP ROLE
  • An advisory lock registry (using namespace key 42)
  • SQL safety requirements for identifier quoting
  • A detailed list of 7 specific issues in the main branch that need fixing

This is a documentation-only PR with no code changes. The spec is intended to guide future implementation work to fix the identified issues.

Review & Testing Checklist for Human

  • Verify the three SQL patterns are technically correct, especially the advisory lock usage with pg_advisory_xact_lock(42, hashtext(...)) and the dual exception handling (duplicate_object OR unique_violation)
  • Confirm all affected code paths were identified - search for any additional CREATE ROLE, CREATE USER, GRANT, REVOKE, DROP ROLE statements that may have been missed
  • Validate the SQL injection concern in removeDbRoles() (lines 139-141 of client.ts) - verify that ${username} interpolation without format('%I', ...) is actually a risk given the upstream sanitization

Notes

  • The advisory lock namespace 42 was chosen arbitrarily since no other advisory locks currently exist in the codebase. Consider if a different convention is preferred.
  • This spec is derived from analyzing PR Fix concurrent role creation race condition #325 which implements these patterns - that PR can serve as a reference implementation.

Link to Devin run: https://app.devin.ai/sessions/94c39fc3c7184ff6b799f00bb5d17782
Requested by: Dan Lynch ([email protected]) / @pyramation

Document the patterns and requirements for creating, granting, and managing
PostgreSQL roles and users to address race conditions, idempotency, and SQL
safety concerns.

Key patterns documented:
- CREATE ROLE with pre-existence check, advisory lock, and dual exception handling
- GRANT role membership with pre-existence check and exception handling
- REVOKE/DROP ROLE with proper identifier quoting

Lists all affected code paths and specific issues to fix in main branch.

Co-Authored-By: Dan Lynch <[email protected]>
@devin-ai-integration
Copy link
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@pyramation pyramation closed this Dec 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants