- Notifications
You must be signed in to change notification settings - Fork15
[ISSUE-7] Added PARALLEL SAFE mark to pgSphere functions#25
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Makefile Outdated
| pg_sphere--1.2.1--1.2.2.sql: upgrade_scripts/pg_sphere--1.2.1--1.2.2.sql.in | ||
| cat $^ > $@ | ||
| ifeq ($(has_parallel), n) | ||
| sed -i -e '/PARALLEL/d' $@ # version $(pg_version) does not have support for PARALLEL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just checking, but shouldn't this line (and lines 120 and 204 and 210) besed -i -e 's/ *PARALLEL SAFE//' $@?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These lines work only for pg versions less than 9.6 (PARALLEL SAFE is not implemented for earlier versions). Now I removed 9.X versions from tests because these versions are not supported by the community. Furthermore, I'm not sure that sed is executed correctly. It removes the whole line, not only parallel safe marks (IMMUTABLE STRICT seems to be removed as well). When I did the patch I just 'copy-pasted' the old solution. I propose to keep my patch unchanged but to rework these lines in another Issue if we still need support of 9.X versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the sed commands should besed -i -e 's/ *PARALLEL SAFE//' $@, but I agree there's no point in continuing to support Pg 9.x, so might as well just remove all of these sed commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@esabol I agree, I think to remove obsolete lines of code. I will do it then.
esabol commentedJul 4, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
By the way, if you put "[ISSUE#7]" in the title of the PR instead of "[ISSUE-7]", GitHub will automatically link this PR to that issue. |
The patch is based on Feodor Sigaev's branch (parallel_safe).Some adjustments were made to fix compilation. Checked the number ofchanges in function declarations and the number of function alterationsin the upgrade script pg_sphere--1.2.1--1.2.2.sql.in (the numbersshould match).Some stuff in Makefile related to 9.X versions (PARALLEL SAFEremoval from install and update scripts) was also removed.
@esabol Thank you for the review! |
The patch is based on Feodor Sigaev's branch (parallel_safe). Some adjustments were made to fix compilation. Checked the number of changes in function declarations and the number of function alterations in the upgrade script pg_sphere--1.2.1--1.2.2.sql.in (the numbers should match).