Fixes for some bugs [in 5.0.0-beta] for which solutions could be found quickly #545
Conversation
* sql/*/*.sql * test/*.sql * updates/pg_partman--4.7.3--5.0.0-beta.sql Trailing whitespaces are redundant and removed by IDE automatically. Gather all these changes in a single commit. Historical files usually left "as is" but could also be slightly reformatted later.
Hard to exploit (need a double quote for extension schema) but it is better to have correct query. Also adjust type of a var: it can never overflow but make linters be quiet. Also change named notation[1] for function arguments to modern style. [1] https://www.postgresql.org/docs/current/sql-syntax-calling-funcs.html#SQL-SYNTAX-CALLING-FUNCS-NAMED
The `wildcard` function already returns sorted files list. https://www.gnu.org/software/make/manual/html_node/Wildcard-Function.html If is is required strictly ordered prerequisites a pipe symbol should be used: https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html
It is useful for update/install files generator three commits later. The old install file is copied from a state as of tag v4.7.3: updates/pg_partman--4.7.3.sql It is useful to have two of them: install file and previous one to check upgrade script. Install the newest is usual: ... # CREATE EXTENSION pg_partman; Test of upgrade can be done via commands: ... # CREATE EXTENSION pg_partman VERSION "4.7.3"; ... # ALTER EXTENSION pg_partman UPDATE;
It allows to have (as far as possible) single point of truth. This commit changes nothing in the current migration file: updates/pg_partman--4.7.3--5.0.0-beta.sql
Split table creation and copying data. The best way to watch changes is: git diff --color-moved HEAD^..HEAD
Gather table's DDL together. It makes the last commit in the series (5/5) be simpler (when tables DDL split into several files). The best way to watch changes is: git diff --color-moved HEAD^..HEAD
Join INSERT & UPDATE to a single UPDATE.
Split single file with table creation DDL into different files.
It will help to use it in the next commit.
Also it helps to compare similar tables, e.g.
git diff \
--word-diff=color \
--word-diff-regex='[a-zA-Z0-9]|[^[:space:]]' \
--no-index \
sql/tables/part_config.sql sql/tables/part_config_sub.sql
The best way to watch changes is:
git diff --color-moved HEAD^..HEAD
Use table creation DDL in generated upgrade script.
Only empty lines, comments and unnecessary type cast ("::regclass")
are changed in the reference file:
updates/pg_partman--4.7.3--5.0.0-beta.sql
Do not add files to Makefile for upgrade script. Now it is only helps to further work; functions will be added later when important changes there are made.
Also changed indent from 2 spaces to 4 spaces in sql/functions/dump_partitioned_table_definition.sql Do not add files to Makefile for upgrade script. Now it is only helps to further work; functions will be added later when important changes there are made. The best way to check here is only whitespace changes is: git diff --word-diff HEAD^..HEAD
* p_default_table_ => p_default_table * partition_schema => partition_schemaname * add: v_parent_schema
Use correct quoting for columns (attributes).
Previous version gave wrong result for arguments like:
SELECT check_name_length(repeat('josé', 15), '12345')::name
|
At this point, I'm not likely going to have the time to evaluate the Makefile changes for the initial 5.0 release. Could you please redo this PR (or make another one) to only be changing the update file? I have my own other method for getting those changes to the individual files for now. |
|
OK, I'd like to open a new PR (with new branch) and support both of them because if I find something strange I'll change this one and reflect changes to another one. It greatly increases my performance, because I do not have your method to spread changes from update file to individual ones. 😇 One question: if changes from this branch can be applied to individual files without conflicts, may I leave them applied or I should create a branch with only "updates/pg_partman--4.7.3--5.0.0-beta.sql" changed? |
Please do not touch any individual files in the PR for now. Thank you! |
|
I noticed the Is it worth to add a commit touched "base" file to make declaration look similar to ones of other functions? |
vitaly-burovoy
force-pushed
the
5.0.0-beta@/fixes_v1
branch
from
b99989a
to
b587fe7
Compare
|
This PR is still marked as |
|
Sorry, that was my mistake. I'll hopefully get more time to look at this next week. |
|
One more question... I'd like to submit some semantic improvements not related to fixing bugs but which increases readability like use IF NOT FOUND THENinstead of IF v_jobmon IS NULL THENafter selecting I guess these changes lead to increase size of "updates/pg_partman--4.7.3--5.0.0-beta.sql" because full function bodies will be there even if a several lines are changed and will be hard to understand why they are there. Is it OK to send this changes here and then decide what to do with them (will be visible changes in sources) or I should create a new branch/PR from some commit not connected with fixing bugs, e.g. b43e488? |
If you could wait for any changes like this that won't be part of 5.0 until 5.0 has been released, I'd appreciate it. Trying to include it with this would just be a bit confusing. Not sure I like that change to NOT FOUND personally. Doesn't really give any benefit that I'm aware of and I think is less clear about what is being used to make the conditional decision. I think the NOT FOUND conditional is better for simpler, one-off query result evaluation, not when it's part of larger plpgsql functions. |
|
Oops. It seems I get wrong words... 😇 I mean whether it is OK to push on top of this branch (I will keep it updated) to decrease possible conflicts or create a new branch from a commit below commits with fixes? P.S.: the query I sent example is quite small: SELECT jobmon INTO v_jobmon FROM @extschema@.part_config WHERE parent_table = p_parent_schema ||'.'|| p_parent_tablename;
IF v_jobmon IS NULL THEN
RAISE EXCEPTION 'Given table is not managed by this extention: %.%', p_parent_schema, p_parent_tablename;
END IF;which actually looks like this: "if nothing was about 'jobmon', then |
|
This seems more a personal preference for the code than any real technical benefit (unless I'm wrong which please clarify then). It just seems clearer what's going on, especially when you don't come back to look at code for long periods of time. And it keeps it consistent with how it is done elsewhere. Please just wait on any style/semantic changes like this until after 5.0.0 is released. Don't get me wrong, I appreciate your contribution, but this is a big change already so I'd prefer stylistic code updates that don't change core functionality to wait until the other bigger work is done. They'll be much easier to review. |
|
OK, I'll wait the release. Style changes can certainly wait. |
…d quickly [port from #545] (#546) * Remove trailing spaces in actual SQL files * sql/*/*.sql * test/*.sql * updates/pg_partman--4.7.3--5.0.0-beta.sql Trailing whitespaces are redundant and removed by IDE automatically. Gather all these changes in a single commit. Historical files usually left "as is" but could also be slightly reformatted later. * Fix detection PG version & update error message * Improve pg_partman extension schema detection Hard to exploit (need a double quote for extension schema) but it is better to have correct query. Also adjust type of a var: it can never overflow but make linters be quiet. Also change named notation[1] for function arguments to modern style. [1] https://www.postgresql.org/docs/current/sql-syntax-calling-funcs.html#SQL-SYNTAX-CALLING-FUNCS-NAMED * Simplify Makefile rule The `wildcard` function already returns sorted files list. https://www.gnu.org/software/make/manual/html_node/Wildcard-Function.html If is is required strictly ordered prerequisites a pipe symbol should be used: https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html * Adjust file name with function name: dump_partitioned_table_definition * Remove unused variables Do not add files to Makefile for upgrade script. Now it is only helps to further work; functions will be added later when important changes there are made. * Make functions declaration look similar Also changed indent from 2 spaces to 4 spaces in sql/functions/dump_partitioned_table_definition.sql Do not add files to Makefile for upgrade script. Now it is only helps to further work; functions will be added later when important changes there are made. The best way to check here is only whitespace changes is: git diff --word-diff HEAD^..HEAD * Fix assignment to different types (add type cast) * Fix absent variable names * p_default_table_ => p_default_table * partition_schema => partition_schemaname * add: v_parent_schema * Fix (delete) extra column from SELECT with no INTO corresponded field * Get rid of unnecessary dynamic SQL in `run_maintenance_proc` * Get rid of some dynamic SQL in `undo_partition` & `partition_data_*` Use correct quoting for columns (attributes). * Fix `check_name_length` with non-ASCII characters Previous version gave wrong result for arguments like: SELECT check_name_length(repeat('josé', 15), '12345')::name * Apply suggestions from code review * Apply suggestions from code review --------- Co-authored-by: Keith Fiske <keith@keithf4.com>
Though some fixes depend on project knowledge, e.g.
sql/procedures/undo_partition_proc.sql:63
v_target_schema,v_target_tablenameare set but never used but should (in branch whenp_target_tableis not set)sql/functions/run_maintenance.sql:54
v_sub_refresh_donenever assigned and used as NULL at row 361;row 367 is useless because
array_appenddoes not append inplace, it returns new array.etc.
The branch is based on previous PoC #544, but still can be rebased on the "5.0.0-beta" branch if get changes from the file "updates/pg_partman--4.7.3--5.0.0-beta.sql" only. Two commits from there are required to reduce whitespace conflicts: