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

Create postgresql_domain module #706

Open
arcturustech opened this issue Jun 14, 2024 · 17 comments
Open

Create postgresql_domain module #706

arcturustech opened this issue Jun 14, 2024 · 17 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@arcturustech
Copy link

arcturustech commented Jun 14, 2024

SUMMARY

This would be a new module to manage domains. Interface would be exactly analogous to postgresql.ext, postgresql.schema, or many others that manage create, drop and alter other major postgres objects

ISSUE TYPE
  • Feature Idea
COMPONENT NAME

postgresql_domain

ADDITIONAL INFORMATION

There is currently no easy or obvious way to manage domains idempotently in ansible playbooks, the way there now is for tables, schemas, extensions, etc.

Domains are a puzzling exception to the wonderful way almost all other postgres objects can be managed in ansible. Without this, a developer will have to fall back on raw queries through the query module, and manage idempotence manually

    - name: Create a COUNT domain in the INVENTORY schema
      become_user: postgres
      community.postgresql.postgresql_domain:
        name: "COUNT"
        db: "WIDGET_INC"
        schema: "INVENTORY"
        base_type: "INT"
        check: "VALUE >= 0"
        default: 0
        cascade: false
        comment: "Domains are a great, underused tool for code readability and maintainability"
        state: present
@Andersson007
Copy link
Collaborator

@arcturustech hello, thanks for opening the issue!
Does a separate module bring any value compared to using just the postgresql_query module?
Specifically, can it provide full idempotency?
I know there are some modules (like postgresql_table, _idx, etc.) that don't work fully idempotently and it's partly my fault due to not being experienced at that time but imo there shouldn't be modules like that.
I'm not a user so why I'm asking this because I don't know.
For example, if we create a domain using the task from the description and later change any of the arguments values, will the module change the object to match the new state or it's impossible w/o destroying the object and re-creating it from scratch?

What do others think?
Thanks

@Andersson007 Andersson007 added the enhancement New feature or request label Jun 17, 2024
@hunleyd
Copy link
Collaborator

hunleyd commented Jun 17, 2024

iirc, renaming a domain is the only altering you can do w/o dropping and recreating

@Andersson007
Copy link
Collaborator

Andersson007 commented Jun 17, 2024

iirc, renaming a domain is the only altering you can do w/o dropping and recreating

@hunleyd thanks! in this case, i would suggest using postgresql_query instead of adding another module because those _table , _idx, etc modules is antipattern

@arcturustech
Copy link
Author

arcturustech commented Jun 17, 2024

iirc, renaming a domain is the only altering you can do w/o dropping and recreating

Actually the postgres ALTER DOMAIN statement (as of the current version 16) is pretty complete, as per the official documentation: https://www.postgresql.org/docs/current/sql-alterdomain.html

As you can see, pretty much every parameter of a DOMAIN can be altered without dropping and recreating. (If that were not true, I would see little point in using domains, but because it is true it makes them highly useful.)

@hunleyd
Copy link
Collaborator

hunleyd commented Jun 18, 2024

I should have been clearer, you cannot change a constraint w/o dropping the existing constraint and adding the new one, and you cannot change the base type w/o dropping the domain itself. You can:

-    { SET DEFAULT expression | DROP DEFAULT }
-    { SET | DROP } NOT NULL
-    ADD domain_constraint [ NOT VALID ]
-    DROP CONSTRAINT [ IF EXISTS ] constraint_name [ RESTRICT | CASCADE ]
-    RENAME CONSTRAINT constraint_name TO new_constraint_name
-    VALIDATE CONSTRAINT constraint_name
-    OWNER TO { new_owner | CURRENT_ROLE | CURRENT_USER | SESSION_USER }
-    RENAME TO new_name
-    SET SCHEMA new_schema

which means of the proposed module args both:

-        base_type: "INT"
-        check: "VALUE >= 0"

cannot be changed, which to me seems to be a the 'base functionality' of the proposed module.

Having said that, I happily defer to others on the matter.

@arcturustech
Copy link
Author

arcturustech commented Jun 18, 2024

It's true that you cannot alter the base type of a domain. Presumably this is because doing so would make no sense, and it's not something a developer should ever want to do. A domain is by definition a subset of another datatype - the base datatype is fundamental to what the domain is and should not be changeable. So I understand why the postgres team did not provide for this and likely never would.

The constraints (note plural) really are the heart of what you need to be able to alter to make domains useful, as they define the subset. Those alterations take two basic forms - contracting the domain subset (by adding a constraint) and expanding the domain subset (by removing a constraint).

Initially, my example has one constraint (value >=0). If I later decide that no COUNT should be too large (restrict the subset), it's true I can't change constraint1 to (value >= 0 and value < 1000) but I can certainly use ALTER DOMAIN to add a second, independent constraint2 (value < 1000). If I then decide that 0 value COUNT is also unacceptable, I simply add constraint3 (value > 0). At that point constraint1 (value >= 0) is still applicable, just always overridden by constraint3. I could then remove constraint1, but I don't have to and probably wouldn't, as it will then still be there doing it's job if I ever remove constraint3. Meanwhile, once I realize that constraint2 was a bad idea, I can remove it independently (expand the subset)

The important thing is that I can massage the constraints (as a group) to provide any subset of the base datatype that I need without having to drop and re-create the domain (and worse yet every table that depends on that domain).

So does this count as having to drop and re-create the domain itself? That would seem to depend on the logic of why drop and re-create would be considered an anti-pattern and whether that logic still applies to this situation. That is something I don't fully understand, and at any rate you will have to decide for yourself.

@hunleyd
Copy link
Collaborator

hunleyd commented Jun 19, 2024

Given the following:

    - name: Create a COUNT domain in the INVENTORY schema
      become_user: postgres
      community.postgresql.postgresql_domain:
        name: "COUNT"
        db: "WIDGET_INC"
        schema: "INVENTORY"
        base_type: "INT"
        check: "VALUE >= 0"
        default: 0
        cascade: false
        comment: "Domains are a great, underused tool for code readability and maintainability"
        state: present

[time passes]

    - name: Create a COUNT domain in the INVENTORY schema
      become_user: postgres
      community.postgresql.postgresql_domain:
        name: "COUNT"
        db: "WIDGET_INC"
        schema: "INVENTORY"
        base_type: "INT"
        check: "VALUE < 1000"
        default: 0
        cascade: false
        comment: "Domains are a great, underused tool for code readability and maintainability"
        state: present

how would the module know that this is an additional constraint and not a desire to replace the existing constraint?

@arcturustech
Copy link
Author

arcturustech commented Jun 19, 2024

That's a good question. It looks like the name of the constraint would have to be a required argument. Sorry, I didn't think of that in my initial proposal.

Another option might be to convert the check: argument from a string to a list, which might be more in line with the interface conventions of other modules

@hunleyd
Copy link
Collaborator

hunleyd commented Jun 20, 2024

Making it a list would make sense. Do you have thoughts @Andersson007 ?

@Andersson007
Copy link
Collaborator

hey folks, great discussion, thanks
So, do i understand correctly that we can add and remove those checks? If yes,
let's say this is an initial value

check: "VALUE >= 0"

then we changed it to

check: "VALUE < 1000"

can it be replaced in DB? i.e. the second one added, the first one removed?
If yes, it sounds like an idempotent thing to me.
@hunleyd you asked how the module determines - same as in, say, any user module: there'll probably be another argument called, say, append_check: true|false with a default value (i'd suggest false, i.e. replace).
Passing a list SGTM

Could you folks please answer my questions

@hunleyd
Copy link
Collaborator

hunleyd commented Jun 24, 2024

@Andersson007 we could just make check into checks and it always be a list?

checks: ['value >=0', 'value < 1000']

and every time the module runs it makes sure the list of checks exists as written?

@Andersson007
Copy link
Collaborator

@Andersson007 we could just make check into checks and it always be a list?

checks: ['value >=0', 'value < 1000']

and every time the module runs it makes sure the list of checks exists as written?

@hunleyd we could also make it of the raw type which can accept a string or a list (any type basically but we ensure in the code that it's either list or str)

@arcturustech
Copy link
Author

arcturustech commented Jun 24, 2024

can it be replaced in DB? i.e. the second one added, the first one removed?
If yes, it sounds like an idempotent thing to me.

Yes, that's true, and agreed I think that is totally idempotent

same as in, say, any user module: there'll probably be another argument called, say, append_check: true|false with a default value (i'd suggest false, i.e. replace).
Passing a list SGTM

When I suggested a list earlier, I totally forgot that each constraint has a name - so actually maybe the even better suggestion for checks would be a dict
checks: { "constraint1": "value >= 0", ... }
And this interface I think would fully cover all possibilities that postgres allows, and also would not require the separate append_check argument

However -

make it of the raw type which can accept a string or a list (any type basically but we ensure in the code that it's either list or str

This is a great suggestion (if we substitute dict for list), it goes beyond the strictly necessary but would be definitely desirable from a convenience and usability standpoint. I suspect that most developers don't and would not want to actually make use of the multiple checks feature, simply provide one check as a simple string, and let it be replaced idempotently via drop and re-create the one constraint, and simply let postgres assign a default name.

Another option to do that might be to provide two arguments: checks (plural), which always takes a dict, for those who want to use the full capability of multiple check constraints, and check (singular) which always takes a string, for those who wish to just use a single constraint which can be idempotently changed by dropping and re-creating. Presumably only check or checks, but not both, should be specified in a single task

Further thoughts, anyone?

@hunleyd
Copy link
Collaborator

hunleyd commented Jun 25, 2024

Another option to do that might be to provide two arguments: checks (plural), which always takes a dict, for those who want to use the full capability of multiple check constraints, and check (singular) which always takes a string, for those who wish to just use a single constraint which can be idempotently changed by dropping and re-creating. Presumably only check or checks, but not both, should be specified in a single task

i kinda like this actually

@Andersson007
Copy link
Collaborator

Another option to do that might be to provide two arguments: checks (plural), which always takes a dict, for those who want to use the full capability of multiple check constraints, and check (singular) which always takes a string, for those who wish to just use a single constraint which can be idempotently changed by dropping and re-creating. Presumably only check or checks, but not both, should be specified in a single task

i kinda like this actually

could get better seen during development:)

@arcturustech would you like to try to create it yourself or anyone else can?

@arcturustech
Copy link
Author

@arcturustech would you like to try to create it yourself or anyone else can?

As much as I would like to contribute, I don't have the time any time soon, so anyone else is most welcome and appreciated to take it on

@Andersson007 Andersson007 added the help wanted Extra attention is needed label Jun 28, 2024
@Andersson007
Copy link
Collaborator

i was hoping to take it myself but have no time now.
one important thing to mention: i would suggest having one option which accepts a string or dict instead of having two arguments OR just always accept only dicts.
justification: make the interface as simple as possible
however whoever will create it it's their business but please take it into consideration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants