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

Fix updates to handle sequences #3339

Merged
merged 1 commit into from Jun 21, 2021
Merged

Conversation

nikkhils
Copy link
Contributor

@nikkhils nikkhils commented Jun 16, 2021

The post-update script was handling preserving initprivs for newly
added catalog tables and views. However, newly added catalog sequences
need separate handling otherwise update tests start failing. We also
now grant privileges for all future sequences in the update tests.

In passing, default the PG_VERSION in the update tests to 12 since we
don't work with PG11 anymore.

@nikkhils nikkhils self-assigned this Jun 16, 2021
@nikkhils nikkhils requested a review from a team as a code owner June 16, 2021 08:15
@nikkhils nikkhils requested review from mfundul and removed request for a team June 16, 2021 08:15
@mkindahl
Copy link
Contributor

The post-update script was handling preserving initprivs for newly
added catalog tables and views. However, newly added catalog sequences
need separate handling otherwise update tests start failing. We also
now grant privileges for all future sequences.

You don't. You only grant privileges for all future sequences in the update test. Not as part of the extension installation, which would be strange.

In passing, default the PG_VERSION in the update tests to 12 since we
don't work with PG11 anymore.

Copy link
Contributor

@mkindahl mkindahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. You need to fix the commit message though so that it doesn't look like the extension sets default privileges for all installations. The change you have is only for the update test.

@nikkhils
Copy link
Contributor Author

LGTM. You need to fix the commit message though so that it doesn't look like the extension sets default privileges for all installations. The change you have is only for the update test.

Ah yes, the commit message suggests that whole sale. Will change

@codecov
Copy link

codecov bot commented Jun 16, 2021

Codecov Report

Merging #3339 (8ee6768) into master (71e8f13) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3339      +/-   ##
==========================================
- Coverage   90.58%   90.57%   -0.02%     
==========================================
  Files         211      211              
  Lines       35536    35536              
==========================================
- Hits        32192    32188       -4     
- Misses       3344     3348       +4     
Impacted Files Coverage Δ
src/loader/bgw_message_queue.c 85.52% <0.00%> (-2.64%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d800e2f...8ee6768. Read the comment docs.

The post-update script was handling preserving initprivs for newly
added catalog tables and views. However, newly added catalog sequences
need separate handling otherwise update tests start failing. We also
now grant privileges for all future sequences in the update tests.

In passing, default the PG_VERSION in the update tests to 12 since we
don't work with PG11 anymore.
@nikkhils nikkhils merged commit 8aaef4a into timescale:master Jun 21, 2021
@nikkhils nikkhils deleted the seq_initprivs branch June 21, 2021 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants