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

Use of special types like regclass in part_config* and functions #540

Open
vitaly-burovoy opened this issue Jun 27, 2023 · 2 comments
Open

Comments

@vitaly-burovoy
Copy link
Contributor

Hello!

It is an interesting project and I have a couple of questions.

I. Are there any reasons to not use [now] special types for appropriate columns instead of just "text"?

  1. regclass for parent_table and template_table
  2. regnamespace for retention_schema
    etc.

You are playing with "split" and "join" without quoting/unquoting those parts and it could lead to unpredictable errors.
Also the use of special types helps to avoid unnecessary explicit joins when you looking up related data in pg_class.
Moreover regclass etc. allow other people to safely (without paying extra attention to @extschema@.part_config{,_sub}) rename objects or move tables to any other schema.

II. Since you refactor the project and change the major version part (which allows breaking change) and it is already in the "beta" are you planning to introduce more changes before release (for example, described in #509 or #511 or questioned above) or this state will be released (with bug fixes and doc improvements)?


P.S.:
Please, for security reasons, NEVER advise [1] other people to create a DDL relation name by concatenating its parts, use the format function for this (just like it is used in the project codebase)!

- SELECT 'ALTER TABLE '||n.nspname||'.'||c.relname||' RENAME TO '||substring(…
+ SELECT format('ALTER TABLE %I.%I RENAME TO %I;', nspname, relname, substring(…

¹ https://github.com/pgpartman/pg_partman/blob/5.0.0-beta/doc/pg_partman_5.0.0_upgrade.md

@keithf4
Copy link
Collaborator

keithf4 commented Jun 28, 2023

I'll have to look into the usage of regclass/regnamespace. Did you mean for function parameters or just more internally in the function.

All features that will be in the initial 5.0.0 release have been tagged as such if there is an issue open. If it is tagged for 5.1, those will be what I look into more immediately for the next release. Issues marked Future are under consideration, but aren't planned for any specific release yet.

Good point on the SQL instructions there. I definitely use format() in the code extensively, so I should stick with that for my examples.

@vitaly-burovoy
Copy link
Contributor Author

Did you mean for function parameters or just more internally in the function.

Of course everywhere. I expect this will greatly simplify the internal code and get rid of some bugs.

All features that will be in the initial 5.0.0 release have been tagged as such

OK... This means changes towards regclass/regnamespace will require the major digit bump (at least 6.0.0): there would be a lot of breaking changes.

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

No branches or pull requests

2 participants