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

Install downgrade scripts by default #6637

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hanefi
Copy link
Member

@hanefi hanefi commented Jan 23, 2023

This commit removes downgrades/ folders and merges their contents with their parents.

Downgrade script mechanism is now working fine for quite a while. I think it makes sense to install all the downgrades scripts by default as well.

This change will potentially solve some issues with downgrades such as citusdata/citus_docs#1070 where a user had troubles downgrading because we started to place some of the downgrade scripts in the usual sql directory and hence we are not able to install all downgrade scripts on make install-downgrades. This PR solves this issue.

Closes citusdata/citus_docs#1070

@hanefi hanefi force-pushed the install-downgrades-by-default branch from 7fab05d to 46b1606 Compare January 23, 2023 19:40
@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #6637 (7377bdf) into main (94b63f3) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #6637   +/-   ##
=======================================
  Coverage   93.01%   93.02%           
=======================================
  Files         259      259           
  Lines       55851    55852    +1     
=======================================
+ Hits        51952    51958    +6     
+ Misses       3899     3894    -5     

@marcocitus
Copy link
Member

Do we need to remove the downgrades/ folder? The separation seems kind of useful given the large number of files in the sql directory.

@hanefi
Copy link
Member Author

hanefi commented Jan 24, 2023

Do we need to remove the downgrades/ folder?

No we do not need to do that. We can keep the folder.

The separation seems kind of useful given the large number of files in the sql directory.

I agree. However, for several reasons we did not place all the downgrade scripts in that folder.

How about keeping downgrades/ and making sure that all the upgrades are in sql/ and all the downgrades are in downgrades/?

@marcocitus
Copy link
Member

How about keeping downgrades/ and making sure that all the upgrades are in sql/ and all the downgrades are in downgrades/?

seems reasonable

@hanefi hanefi force-pushed the install-downgrades-by-default branch from 46b1606 to f34ac23 Compare January 25, 2023 11:57
@hanefi
Copy link
Member Author

hanefi commented Jan 25, 2023

I removed all install-all targets as there is no need for them now.

I intentionally left install-downgrades target as it is. Older versions of Citus can still use that make target to install downgrade scripts in their clusters using that one.

I suggest we complete this PR after 11.2 release is over.

@hanefi hanefi marked this pull request as ready for review January 25, 2023 12:49
@JelteF
Copy link
Contributor

JelteF commented Jan 30, 2023

So what are the downgrade steps that people are supposed to follow? Is it this?

  1. notice upgrade caused problems
  2. run ALTER EXTENSION citus UPDATE 11.1-3
  3. install 11.1 system packages

@hanefi
Copy link
Member Author

hanefi commented Jan 30, 2023

So what are the downgrade steps that people are supposed to follow? Is it this?

  1. notice upgrade caused problems
  2. run ALTER EXTENSION citus UPDATE 11.1-3
  3. install 11.1 system packages

This list of actions is not possible without SET citus.enable_version_checks TO 'false';. I am not sure if we want an open source to mess around with this GUC.

I think it is still safer to do the following instead:

  1. notice upgrade caused problems.
  2. install 11.1 system packages
  3. get a copy of the latest source code, and run make install-downgrades
  4. Run ALTER EXTENSION citus UPDATE without supplying any versions.

The end user does not (and IMO should not) really care about the migration version, and they can easily break their systems if they migrate to a wrong version. For example, 11.1.5 uses migration version of 11.1-1. We do not even have the migration version 11.1-3 that you have in step 2.

A better solution would be maintaining a downgrade package that installs the latest downgrade scripts. If we had such a package, we could prefer the following actions:

  1. notice upgrade caused problems. (you have the citus package and latest downgrade package as it is listed as a dependency)
  2. install 11.1 system packages. (installing 11.1 packages do not touch the existing downgrade package installed in the system as the dependency is still satisfied)
  3. run ALTER EXTENSION citus UPDATE without supplying any versions.

@JelteF
Copy link
Contributor

JelteF commented Feb 7, 2023

I think it is still safer to do the following instead:

  1. notice upgrade caused problems.
  2. install 11.1 system packages
  3. get a copy of the latest source code, and run make install-downgrades
  4. Run ALTER EXTENSION citus UPDATE without supplying any versions.

I think @marcocitus found some problem with this flow a while back, and that it didn't actually work in practice. And we never noticed because our tests don't actually do it, but instead use the SET citus.enable_version_checks TO 'false'; trick. @marcocitus could you share some context on this if you have that?

I would really like a flow that allows you to run ALTER EXTENSION citus UPDATE.

For context, the reason why downgrades from the downgrades directory are not installed by default is because we indeed had this citus-downgrades package plan (but so far we never did it). And if the files would be part of the main package, they would conflict with the ones from the downgrade package.

@marcocitus
Copy link
Member

Iirc the columnar decoupling in 11.0 -> 11.1 necessitates that we do the ALTER EXTENSION citus UPDATE TO '11.0-4 from the 11.1 binaries, because it uses the utility hook to modify the behaviour of ALTER EXTENSION.

There was an issue downgrading from 11.0 to 10.2 #6041, though that was using 10.2 binaries for running the ALTER EXTENSION.

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.

Downgrade citus gives error.
3 participants