Fixed problems with per database extensions and schemas #140 #223
Conversation
postgres/manage.sls
Outdated
|
|
||
| {%- if 'extensions' in db %} | ||
| {%- for ext_name, extension in db.pop('extensions')|dictsort() %} | ||
| {%- do extension.update({'maintenance_db': name }) %} |
There was a problem hiding this comment.
If we'd look at CREATE EXTENSION PG SQL syntax, we see that there is no direct relation between an extension and a database.
So an extension is intended to be installed into a schema, and in turn a schema should be associated with a database.
The maintenance_db parameter for all postges_*.present state function means PG cluser-wide database containing PostgreSQL internal entities, this is not the database you could install extension to.
So, no need to for the line above (49).
There was a problem hiding this comment.
Following salt docs for postgresql_extension.present:
maintenance_db
Database to act on
I've checked and state behave as docs describe. Schema is optional for 'CREATE EXTENSION', default is to create extension for current database.
There was a problem hiding this comment.
Oh, yes. You're absolutely right. An extension state is little bit different from the others in this case. I have checked the code of postgres.psql_query function (which is the workhorse of all those states) and it simply substitutes maintenance_db to psql --dbname {{maintenance_db}} raw command.
That's quite confusing, because other functions use dbname parameter instead of maintenance_db, so I misunderstood them. Thanks for pointing me to it.
There was a problem hiding this comment.
Oh, yes. You're absolutely right. An extension state is little bit different from the others in this case. I have checked the code of postgres.psql_query function (which is the workhorse of all those states) and it simply substitutes maintenance_db to psql --dbname {{maintenance_db}} raw command.
That's quite confusing, because other functions use dbname parameter instead of maintenance_db, so I misunderstood them. Thanks for pointing me to it.
postgres/manage.sls
Outdated
| {%- if 'extensions' in db %} | ||
| {%- for ext_name, extension in db.pop('extensions')|dictsort() %} | ||
| {%- do extension.update({'maintenance_db': name }) %} | ||
| {{ format_state( name + '-' + ext_name, 'postgres_extension', extension) }} |
There was a problem hiding this comment.
Would you mind to separate format_state macro call and related parameters from other Jinja flow control operators?
This would much improve the code readability.
There was a problem hiding this comment.
Maybe you consider to put newlines between state generation code and Jinja loops?
postgres/manage.sls
Outdated
| - require: | ||
| - postgres_database: postgres_database-{{ name }} | ||
| {%- endfor %} | ||
| {%- endif %} |
There was a problem hiding this comment.
Following the logic of raw SQL command, we should first create a database, then schema and finally install an existing extension. You could pop up per-DB schemas and extensions parameters from Pillar dictionary for a single database ahead, and then construct the states afterwards in a logical order.
There was a problem hiding this comment.
Logic is OK. States are executed in correct order. Arrangement in file is irrelevant.
There was a problem hiding this comment.
Yep. Requisite parameters simply put them in right order. That's OK.
postgres/manage.sls
Outdated
| - require: | ||
| - postgres_database-{{ dbname }} |
There was a problem hiding this comment.
This addition would not work properly, because there are no dbname Jinja variable was ever assigned.
You wanted to put schema.dbname, right?
|
@vutny can you take a second look... |
| {%- do kwarg.update({'name': name}) %} | ||
| {%- if 'name' not in kwarg %} | ||
| {%- do kwarg.update({'name': name}) %} | ||
| {%- endif %} |
There was a problem hiding this comment.
This is just awesome. Would mind to update pillar.example file for the formula users to see they now able to provide unique IDs for PG entities?
There was a problem hiding this comment.
You can't set state id via pillar. So no need for pillar.example update. This feature is for formula developers only.
IMHO Allowing user setting state ids is asking for trouble.
There was a problem hiding this comment.
@vutny Nice to hear that.
| {%- do kwarg.update({'name': name}) %} | ||
| {%- if 'name' not in kwarg %} | ||
| {%- do kwarg.update({'name': name}) %} | ||
| {%- endif %} |
|
@aboe76 This PR is ready to merge, thanks. |
Fixes issue #140