Skip to content

Use pg_temp to store PostgreSQL auto provisioning procedures#44255

Merged
gabrielcorado merged 6 commits intomasterfrom
gabrielcorado/postgres-auto-users-pg-temp
Jul 18, 2024
Merged

Use pg_temp to store PostgreSQL auto provisioning procedures#44255
gabrielcorado merged 6 commits intomasterfrom
gabrielcorado/postgres-auto-users-pg-temp

Conversation

@gabrielcorado
Copy link
Copy Markdown
Contributor

@gabrielcorado gabrielcorado commented Jul 16, 2024

Related to #37609.

This moves the auto user provisioning procedures to the temporary schema pg_temp, similar to what is used for database access controls.

Note: This doesn't update how procedures are stored for RedShift (they are still created without specifying the schema). This will be done in a separate PR.

In addition, the tests now also check the procedure presence (asserting per session in case of pg_temp usage) and the argument count on the calls.

changelog: Moved PostgreSQL auto provisioning users procedures to pg_temp schema.

@gabrielcorado gabrielcorado self-assigned this Jul 16, 2024
@github-actions
Copy link
Copy Markdown
Contributor

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@github-actions github-actions Bot requested a review from jakule July 16, 2024 00:13
@github-actions github-actions Bot added database-access Database access related issues and PRs size/md labels Jul 16, 2024
Comment thread lib/srv/db/postgres/users.go Outdated
Comment thread lib/srv/db/postgres/users.go Outdated
Comment thread lib/srv/db/postgres/test.go Outdated
Comment on lines +451 to +457
procName := storedProcedureName(pid, match[storedProcedureRe.SubexpIndex("Schema")], match[storedProcedureRe.SubexpIndex("ProcName")])
var argsCount int
args := strings.Split(match[storedProcedureRe.SubexpIndex("Args")], ",")
for _, arg := range args {
// Skip arguments that have a default value.
if !strings.Contains(strings.ToLower(arg), "default") {
argsCount++
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On one hand, it's quite impressive to validate things like this, but on the other hand, I feel it's too much custom mock logic. I would prefer something simpler but i don't mind keep this either.

What's more important is we cover these in our E2E tests. I see both RDS and Redshift auto-user provisioning passed so we are good 👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on that topic, could you check if E2E tests need to be updated (like do we need to adjust the permissions given to the admin user)?

Comment thread lib/srv/db/postgres/test.go
@gabrielcorado gabrielcorado enabled auto-merge July 18, 2024 15:39
@gabrielcorado gabrielcorado added this pull request to the merge queue Jul 18, 2024
Merged via the queue into master with commit 0daeb99 Jul 18, 2024
@gabrielcorado gabrielcorado deleted the gabrielcorado/postgres-auto-users-pg-temp branch July 18, 2024 18:47
@public-teleport-github-review-bot
Copy link
Copy Markdown

@gabrielcorado See the table below for backport results.

Branch Result
branch/v16 Create PR

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

Labels

database-access Database access related issues and PRs size/md

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants