Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add trusted option in the control file #1397

Merged

Conversation

liuxueyang
Copy link
Contributor

@liuxueyang liuxueyang commented Nov 17, 2023

trusted is supported in postgresql 13 and above. Extensions can add it to the control file and also want to support postgresql 12 and below. This PR removes the trusted option in the control file when installing the extension. The default value of trusted is false.

Related PR in postgresml postgresml/postgresml#810

Options in the control file: https://www.postgresql.org/docs/13/extend-extensions.html#id-1.8.3.20.11

Comment on lines 5 to +6
superuser = false
trusted = false
Copy link
Member

Choose a reason for hiding this comment

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

In a case where this was, e.g.

superuser = false
trusted = true

this would do nothing. While Postgres notes this is simply "irrelevant", I think pgrx should error, or at least warn on that case, rather than permitting it to pass by unremarked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I added another type of error for this case.

Comment on lines 90 to 93
trusted: temp
.get("trusted")
.ok_or(ControlFileError::MissingField { field: "trusted" })?
== &"false",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should require this parameter to be set. We should simply treat the absent case as equivalent to false, since it de facto is.

Copy link
Member

Choose a reason for hiding this comment

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

Doing that would also fix the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. This field is optional now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CI stuck for more than 2 hours. 😢

Copy link
Member

Choose a reason for hiding this comment

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

whoops. accidentally out of budget.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Okay, the formatting problem is fixed on develop, and this otherwise looks fine.

@workingjubilee
Copy link
Member

Thanks!

`trusted` is supported in postgresql 13 and above. Extensions can add it
to control file and also want to support postgresql 12 and below. This
PR removes the trusted option in the control file when installing the
extension. The default value of `trusted` is false.
return error if superuser is false and trusted is true.
@workingjubilee workingjubilee merged commit 6b5da2b into pgcentralfoundation:develop Nov 27, 2023
8 checks passed
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