pgRouting / pgrouting

Repository contains pgRouting library. Development branch is "develop", stable branch is "master"
https://pgrouting.org
GNU General Public License v2.0
1.12k stars 364 forks source link

Missing Extension upgrade from 2.0.0 to 2.1.0 script #372

Closed woodbri closed 8 years ago

woodbri commented 8 years ago

There was a discussion on postgis about upgrading and that reminded me that we probably need a an upgrade from 2.0.0 to 2.1.0 script. For an example look at /usr/share/postgresql/9.4/extension/postgis--2.0.0--2.1.7.sql. @robe2 might be able to help with this.

dkastl commented 8 years ago

Do we introduce breaking changes with 2.1? Not sure if an update script is necessary this time.

Does ALTER EXTENSION pgrouting UPDATE automatically if it's installed as extension, or do we need to test this somehow?

cvvergara commented 8 years ago

I am reading / trying to understand how the update script should look like for pgRouting using ALTER EXTENSION

woodbri commented 8 years ago

I don't know the details of how this works, but @cvvergara and I discussed it and she is looking into it. The issue is that if you wrote a stored procedure that is dependent on on pgRouting, like a wrapper to simplify a web request, and you want to upgrade the database, you need the upgrade script because it does an in place upgrade of the extension. If you were to drop the old extension and create the new one, then the drop would also drop your stored procedure. @robe2 is also knowledgeable in this can can help if @cvvergara gets stuck. I think we should be able to model our script after what postgis does and ours should be simpler.

cvvergara commented 8 years ago

There is this develop-2_0_1 branch, Does this problem was worked/solved on that branch?

cvvergara commented 8 years ago

@robe2 Just to double check, https://github.com/pgRouting/pgrouting/releases/tag/v2.0.0 has this https://github.com/pgRouting/pgrouting/commit/d6ed2cba4c574b8ac463632df451b6f011fd522d commit https://github.com/pgRouting/pgrouting/tree/master branch that has a different commit, but non of the installation/ code / sql files are affected: https://github.com/pgRouting/pgrouting/compare/v2.0.0...master So I need to see what is installed by master what is installed by develop Check what sql functions / types / etc changes And create a postgreSQL upgrade file that handles those changes. Preferably using cmake, but the worse case scenario is to do it manually.

woodbri commented 8 years ago

No, I created the branch for 2_0_1 in case we had fixes that needed to be released but I'm not sure that anything ever got checked into it. The update script can only happen when you know how the release has changed or if it has changed.

robe2 commented 8 years ago

@cvvergara You actually only really need to worry about type changes. As I recall when building upgrade scripts for PostGIS, I could get away with just reinstalling the functions, though I forget if I had to drop them from extension first.

cvvergara commented 8 years ago

@robe2 I found this helper, they are the first functions in the migrating scripts. Will those be useful?

woodbri commented 8 years ago

I'm writing a perl script that will read and parse the sql extension and then compare the two versions, so like for pgrouting--2.0.0.sql, I'm getting:

$funcs = [
           'pgr_gettablename(tab,sname,tname)',
           'pgr_getcolumnname(text,text)',
           'pgr_iscolumnintable(text,text)',
           'pgr_quote_ident(text)',
           'pgr_versionless(text,text)',
           'pgr_startpoint(geometry)',
           'pgr_endpoint(geometry)',
           'pgr_pointtoid(geometry,doubleprecision,text,integer)',
           'pgr_createtopology(text,doubleprecision,text,text,text,text,text)',
           'pgr_createverticestable(text,text,text,text,text)',
           'pgr_analyzegraph(text,doubleprecision,text,text,text,text,text)',
           'pgr_analyzeoneway(text,text[],text[],text[],text[],boolean,text,text,text)',
           'pgr_nodenetwork(text,doubleprecision,text,text,text)',
           'pgr_astar(text,integer,integer,boolean,boolean)',
           'pgr_bdastar(text,integer,integer,boolean,boolean)',
           'pgr_dijkstra(text,integer,integer,boolean,boolean)',
           'pgr_kdijkstracost(text,integer,integerarray,boolean,boolean)',
           'pgr_kdijkstrapath(text,integer,integerarray,boolean,boolean)',
           'pgr_bddijkstra(text,integer,integer,boolean,boolean)',
           'pgr_apspjohnson(text)',
           'pgr_apspwarshall(text,boolean,boolean)',
           'pgr_trsp(text,integer,integer,boolean,boolean,text)',
           'pgr_trsp(text,integer,float8,integer,float8,boolean,boolean,text)',
           'pgr_tsp(float8[][],integer,integer,seq,id)',
           'pgr_makedistancematrix(text,dmatrix,ids)',
           'pgr_tsp(text,integer,integer)',
           'pgr_ksp(text,integer,integer,integer,boolean)',
           'pgr_drivingdistance(text,integer,float8,boolean,boolean)',
           'pgr_alphashape(text,x,y)',
           'pgr_pointsaspolygon(varchar)'
         ];

I'm hoping I can take two versions and compare then do do whatever I need to to create the upgrade script. I'll work on this more tomorrow.

cvvergara commented 8 years ago

@woodbri Thanks Steve.

robe2 commented 8 years ago

@cvvergara Yes those should help, but depending on what you are changing, you might not need to bother.

The postgis_extension_remove_objects function blindly removes all objects of a particular type from the extension:

e.g. in my tiger extension I do this:

SELECT postgis_extension_remove_objects('postgis_tiger_geocoder', 'FUNCTION');
SELECT postgis_extension_remove_objects('postgis_tiger_geocoder', 'AGGREGATE');

It's particularly useful for aggregates since there is no create or replace for aggregates, so I usually just blindly drop them and recreate them.

This is mostly so I don't have to worry about handling drops specially in my script. If you are only adding new functions and not changing the signature of a function (e.g. having to drop them), then you can actually skip this whole step, since the CREATE OR REPLACE function ... will only add a function to an extension if its not already a part of it but will allow you to change the body of the function all you want. DROP it won't let you drop an object that is part of an extension (thus the need to remove it from the extension before you can drop it).

Suggestion -- start with your install script, and simply rename it pgrouting--2.0.0--2.1.0.sql

That will get you about 90% where you need to be, then in that script, remove all the CREATE TYPE and CREATE AGGREGATE stuff that existed in 2.0 that is not changing. If you didn't add new types and aggregates between the two versions, you are fine. If you did, then you need to keep the new ones in and might need to do a drop, create for them if you changed signature.

That will allow you to get a feel for how the extension machinery works and then you can write a translation script that converts your regular install script to an update script.

But having a pgrouting-2.0.0-2.1.0.sql script is all you need to do an upgrade and you might not even need to do any more than what I described if you have no obsolete signatures to drop.

woodbri commented 8 years ago

Ok, I have gotten this far with the script.

My analysis of 2.0.0--2.1.0 is that:

0       pgr_alphashape(text,out float8,out float8)
1       pgr_analyzeoneway(text,text[],text[],text[],text[],boolean,text,text,text)
1       pgr_analyzegraph(text,double precision,text,text,text,text,text)
1       pgr_apspjohnson(text)
1       pgr_apspwarshall(text,boolean,boolean)
1       pgr_astar(text,integer,integer,boolean,boolean)
1       pgr_bdastar(text,integer,integer,boolean,boolean)
1       pgr_bddijkstra(text,integer,integer,boolean,boolean)
0       pgr_createtopology(text,double precision,text,text,text,text,text)
1       pgr_createverticestable(text,text,text,text,text)
0       pgr_dijkstra(text,integer,integer,boolean,boolean)
0       pgr_drivingdistance(text,integer,float8,boolean,boolean)
1       pgr_endpoint(geometry)
1       pgr_getcolumnname(text,text)
1       pgr_gettablename(in text,out text,out text)
1       pgr_iscolumnintable(text,text)
1       pgr_kdijkstracost(text,integer,integer array,boolean,boolean)
1       pgr_kdijkstrapath(text,integer,integer array,boolean,boolean)
1       pgr_ksp(text,integer,integer,integer,boolean)
1       pgr_makedistancematrix(text,out double precision[],out integer[])
1       pgr_nodenetwork(text,double precision,text,text,text)
0       pgr_pointtoid(geometry,double precision,text,integer)
0       pgr_pointsaspolygon(varchar)
1       pgr_quote_ident(text)
1       pgr_startpoint(geometry)
1       pgr_trsp(text,integer,float8,integer,float8,boolean,boolean,text)
1       pgr_trsp(text,integer,integer,boolean,boolean,text)
1       pgr_tsp(float8[][],integer,integer,out integer,out integer)
1       pgr_tsp(text,integer,integer)
1       pgr_versionless(text,text)

where first column says if this 2.0.0 function signature exists in 2.1.0

woodbri commented 8 years ago

I just pushed a new script into develop tools/build-upgrade-scripts

you can run it like "tools/build-upgrade-scripts develop" and it will checkout the 2.0.0, run cmake to generate the build/lib/pgrouting--2.0.0.sql and checkout develop and create the build/lib/pgrouting--2.1.0.sql and create a new file build/lib/pgrouting--2.0.0--2.1.0.sql which I think should be close to the upgrade script we need

I have not had time to actually try and upgrade a database so it needs some more testing.

cvvergara commented 8 years ago

@woodbri just confirming, the changes you made are in develop and not in develop_2_1?

cvvergara commented 8 years ago

What shal I put in the [<cmake-options>]

Usage: build-upgrade-scripts <current_branch> [<cmake-options>]
woodbri commented 8 years ago

Correct, not in develop_2_1_0 I need to think about how to integrate this script into the build system, because it does stuff like git checkout and runs cmake from within the script so I'm not use if is callable from cmake.

cvvergara commented 8 years ago

@sanak @robe2 Can you verify that we get the same results doing the following steps? (I already have a build folder so to make sure things don't get mixed up I remove it first)

git checkout master
rm -rf build
mkdir build
cd build
cmake -DWITH_DOC=ON ..
sudo make install
cd ..
git checkout develop
tools/build-upgrade-scripts develop
tools/test-runner.pl

And got this expected results (because signatures were erased)

           'z_crash' => 0,
           'z_fail' => 50,
           'z_pass' => 43
         };
sanak commented 8 years ago

@cvvergara Okay, I will check it on my Mac Homebrew environment, later (probably, 9 hours later).

cvvergara commented 8 years ago

@woodbri What if... instead of doing the actual removal, it does creates a file containing the removal instructions under src/upgrade_files/sql/v2.0-v2-1.sql

then the upgrade script would be something like concatenating the the contents of the v2.0-v2-1.sql with the contents of the installation file.

That way the

  1. the file would already be in the tree at the time cmake is run.
  2. Linux / Mac / windows users don't need to run the tool.
cvvergara commented 8 years ago

@sanak thanks

woodbri commented 8 years ago

A couple of thoughts on this. I'm not sure what is happening with:

git checkout master
rm -rf build
mkdir build
cd build
cmake -DWITH_DOC=ON ..
sudo make install
cd ..
git checkout develop
tools/build-upgrade-scripts develop
tools/test-runner.pl

because running tools/build-upgrade-scripts after sudo make install should have no impact on the installed files. In fact, the script should not do anything to the existing system other than to write a new file that is ignored into build/lib/. So I'll have to look at that.

The new script has to be generated before make install and needs to be installed with the other install files, but I haven't worked that out yet.

I'm thinking that a better way to do this is to create a signatures-.txt that gets saved in the src tree and then these can be read to generate the upgrade scripts without having to checkout the the tags and running cmake. This will also make it easier to generate the update scripts within the cmake process. So there is more to do here.

robe2 commented 8 years ago

Yes going with a signature tag is better, than trying to checkout different branches. Also the whole checkout exercise sounds very unmanageable once you get to 2.2.

Why can't you just drop all functions from the extension (that is what postgis does) and then just recreate with CREATE OR REPLACE (which your install script does already)

Then like I said the only thing you really need to keep track of are: 1) CREATE TYPE, CREATE AGGREGATE that are new in a release. 2) Function drops for cleanliness you could just keep a script that DROP FUNCTION IF EXISTS .. that should not be in the newer release. That is again something we have built into postgis scripts -- to drop functions that are obsolete.

With that you really don't need to keep track of which version a function was dropped in.

woodbri commented 8 years ago

So what my script does is get the signatures for 2.0.0 and 2.1.0 and then looks for functions that need to be dropped because they don't exist or changed signatures in the newer release. And for TYPEs that that already exist in the new release and comments them out of the new release script. We don't have to deal with AGGREGRATEs at the moment. TODO: is handle TYPEs that exist by name but have changed.

@robe2 During the update process are cascade drops suspended? What happens if you drop a function/type that has dependences? like a user has written a function that depends on a function/type you want to drop?

so my plan is to create file signatures-2.0.0.txt based on 2.0.0 and save it in the tree. The when cmake is run it will create signatures-.txt which should get saved in the tree at some point. then it will find all the old signatures-.txt stored in the tree from past versions and generate update scripts for each to the

The correct method for testing these will be:

  1. create a database
  2. run create extension pgrouting with version '<old-version>';
  3. run alter extension pgrouting update to '<current-version>';
  4. run tests on this database
sanak commented 8 years ago

@cvvergara About current "build-upgrade-scripts.sh", Mac OSX Homebrew environment result was same as your result. FYI

cvvergara commented 8 years ago

@sanak Thanks for the information I wanted to check that the other OS can run the tool.

git checkout master
rm -rf build
mkdir build
cd build
cmake -DWITH_DOC=ON ..
sudo make install      <------  this installs the 2.0
cd ..
git checkout develop    <------- chenging to the 2.1
tools/build-upgrade-scripts develop  <------ this I thought is erasing the functions in 2.0 that are not in 2.1
tools/test-runner.pl   <----- this tests whatever is being installed

I thought it was erasing them, because I didn't see a change using git status.
I misunderstood this instructions: " you can run it like "tools/build-upgrade-scripts develop" and it will checkout the 2.0.0, run cmake to generate the build/lib/pgrouting--2.0.0.sql and checkout develop and create the build/lib/pgrouting--2.1.0.sql and create a new file build/lib/pgrouting--2.0.0--2.1.0.sql which I think should be close to the upgrade script we need "

woodbri commented 8 years ago

I'm still working on these scripts and I'm changing the way things work in a major way, so not ready for testing yet. Also, test-runner.pl creates a new database with the current version of the software based on what has already been installed and it does not look at what is in the build directory. So I will have to modify or create a new script to install an old version of pgrouting and then do the update to the current version and then run the tests in that database.

Regarding your test script above: you install 2.0, checkout develop and run tests based on 2.1 which is why it fails. tools/build-upgrade-scripts develop only writes a file in the build dir, it does not change install anything or modify the server.

You can partially test it like this:

git checkout master
rm -rf build
mkdir build
cd build
cmake -DWITH_DD=YES ..
make
sudo make install
git checkout develop
rm -rf ../build/*
cmake ..
make
sudo make install
../tools/build-upgrade-scripts develop
sudo cp lib/pgrouting--2.0.0--2.1.0.sql /usr/share/postgresql/9.4/extension/
createdb -U postgres -h localhost test_2_0_update
psql -U postgres -h localhost test_2_0_update -c "create extension pgrouting with version '2.0.0'"
psql -U postgres -h localhost test_2_0_update -c "alter extension pgrouting update to '2.1.0'"
psql -U postgres -h localhost test_2_0_update # run some manual tests
woodbri commented 8 years ago

I think I have a working system built to deal with extension version updates.

[develop.] ~/work/pgrouting/build$ ../tools/test-update.sh
CREATE EXTENSION
CREATE EXTENSION
                   pgr_version
-------------------------------------------------
 (2.0.0,pgrouting-2.0.0,0,f26831f,master,1.54.0)
(1 row)

ALTER EXTENSION
                       pgr_version
---------------------------------------------------------
 (2.1.0,pgrouting-2.1.0-alpha1,2,f76bdfc,develop,1.54.0)
(1 row)
woodbri commented 8 years ago

Here is how the new system works:

When we make a new release we need to save the new signature file into tools/sigs/ so it will be available for future releases to generate update scripts.

I ran into a problem of having "develop" which was called 2.0.0 prior to the tag and could not update that version because some of the function sigs had changed. I think we should move to the even odd number scheme to help keep these straight, where even numbered releases like 2.0, 2.2, 2.4, ... are official releases and odd numbered non-releases like 2.1, 2.3, 2.5, ... are interim development non-release numbers. this would help identify if a system is using an official release of some develop code from git.

cvvergara commented 8 years ago

what about travis?

cvvergara commented 8 years ago

Jenkins is also not working.

woodbri commented 8 years ago

Does it work on your system? It works here? :)

Travis:

-- Creating lib/pgrouting--2.1.0.sig

Can't stat ../tools/sigs: No such file or directory

 at /home/travis/build/pgRouting/pgrouting/tools/build-extension-update-files line 35

https://github.com/pgRouting/pgrouting/tree/develop/tools/sigs file is there, so not sure why it has heartburn :(

cvvergara commented 8 years ago

It is not clear to me what is the sequence of instructions I have to give.

woodbri commented 8 years ago
git checkout develop
git pull
rm -rf build
mkdir build
cd build
cmake ..
make
sudo make install
cd ..
tools/test-runner.pl

All of that should work like before. If you want to test the update then:

git checkout pgrouting-2.0.0
rm -rf build
mkdir build
cd build
cmake -DWITH_DD=YES ..
make
sudo make install
git checkout develop
tools/test-update.sh
woodbri commented 8 years ago

travis is fixed.

cvvergara commented 8 years ago

How would the update work for a packaging system?

cvvergara commented 8 years ago

With port 5434, it didn't ran, but I edited it to be port 5432, (not commiting) and it worked on my computer. I also used it in an existing data base, with 2.0.0 and did the following test, tried the a signature of 2.1 in the 2.0, (of course, doesnt find it)

doc_data=# select pgr_version();
                   pgr_version                   
-------------------------------------------------
 (2.0.0,pgrouting-2.0.0,0,f26831f,master,1.55.0)
(1 row)

doc_data=# \i many_to_many.test.sql                        
psql:many_to_many.test.sql:10: ERROR:  function pgr_dijkstra(unknown, integer[], integer[]) does not exist
LINE 1: SELECT * FROM pgr_dijkstra(
                      ^
HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
psql:many_to_many.test.sql:16: ERROR:  function pgr_dijkstra(unknown, integer[], integer[], boolean) does not exist
LINE 1: SELECT * FROM pgr_dijkstra(

made the update, and the function shows.

doc_data=# alter extension pgrouting update to '2.1.0';    
ALTER EXTENSION
doc_data=# \i many_to_many.test.sql                    
 seq | start_v | end_v | node | edge | cost | agg_cost 
-----+---------+-------+------+------+------+----------
   0 |       2 |     3 |    2 |    4 |    1 |        0
   1 |       2 |     3 |    5 |    8 |    1 |        1
... etc
cvvergara commented 8 years ago

@sanak In develop_2_1_0, I merged @woodbri update scripts code, can you please test in MAC? Thank you

woodbri commented 8 years ago

@cvvergara I just push another change and I think I now have both winnie and travis working. You can merge this with develop_2_1_0 again. I'm done for the day. I'll look updating the the test script to make it easier to test the update stuff. At the moment the system seems to be working to generate and install the update scripts.

cvvergara commented 8 years ago

@sanak Please don't do the test because @woodbri is not stable yet in the scripts he is working on. He is making changes in develop's CMakeList and I misinterpreted that he had reached an stable point, which was not the case. and I need to revert the merge, to go back to my stable point and wait for @woodbri to finish his changes.

sanak commented 8 years ago

@cvvergara Okay, thanks for information.

cvvergara commented 8 years ago

@sanak develop and develop_2_1_0 are being merged, with the changes in installation files, Travis tests and Jenkins tests have passed, so, I think you can test now. Use develop branch to make your tests During next week I will be working on the documentation, only changes in the documentation I will be doing but that is going to take place in develop_2_1_0. Thanks.

sanak commented 8 years ago

@cvvergara Okay, thanks for information. I will try to test it about 9 hours later.

cvvergara commented 8 years ago

@sanak Thank you. @dkastl & @sanak In the build guide I really would like to see Nagase San 's efforts reflected in a section for MAC. Can you please post here the contents I need to write?

cvvergara commented 8 years ago

@robe2 Can you please check that the installation instructions are ok for Windows? (also on the build guide ) You can post here the contents missing.

robe2 commented 8 years ago

@cvvergara At a glance that looks fine, there are so many switches you'd need to throw in for building on windows (i imagine other platforms as well, that a link to section where someone has done full detailed instructions is better) than putting in the docs.

For example for building on mingw I have this gist (which I promise I will update before 2.1 is released :) ) - perhaps include a link to that for those who need more detailed and really want to build themselves https://gist.github.com/robe2/5935101#file-makepgroutingdependencies-sh

Most people are just going to get pgRouting via a package, so I think the user instructions are the most important to focus on:

https://github.com/pgRouting/pgrouting/blob/develop_2_1_0/doc/src/installation/index.rst

1) And that I think needs a bit of work. First of all the windows experimental builds are for 9.2-9.4 (released), and probably by the time you release, will have 9.5 in there as well.

2) The Redhat/Fedora/CentOS link is broken - RHEL/CentOS/Fedora and I think that's not the best source anyway. I would use the PostgreSQL Development group (PDG) yum repository: I know they carried pgrouting at least 2.0, they probably haven't gotten to updating for 2.1, but we can shout out to Devrim (PDG yum maintainer) once we are ready for him to test that out for packaging and he can also test against 9.5 as well (which we haven't started doing I imagine). Planning to setup winnie to do that (but I usually wait till EDB releases their beta before I start doing that since they put wieird stuff in there that I got to account for when mixing postgresql vc++ build with mingw)

http://yum.postgresql.org/

3) The install instructions should really cover basic pgRouting install in a database.

Kind of what I have written here: http://www.postgresonline.com/journal/archives/329-An-almost-idiots-guide-to-install-PostgreSQL-9.3,-PostGIS-2.1-and-pgRouting-with-Yum.html

and here: (someone updted the instructions for 9.4 so the link is a bit misleading :) ) https://trac.osgeo.org/postgis/wiki/UsersWikiPostGIS21UbuntuPGSQL93Apt

4) Almost forgot, we'll need to add upgrade instructions

sanak commented 8 years ago

@cvvergara I checked develop branch, and its build was no problem, but about its test, the following 2 tests were failed on my Mac Homebrew environment. I guess that utilities-any.test.sql failure was no problem, but nodeNetwork-any.test.sql failure may be some problem.

           'doc/test/test.conf' => [
                                     {
                                       :
                                       'doc/test/nodeNetwork-any.test.sql' => 'FAILED: 19d18
< 14|2',
                                       :
                                       'doc/test/utilities-any.test.sql' => 'FAILED: 1c1
< 2.1.0|pgrouting-2.1.0
---
> 2.1.0|pgrouting-2.1.0-alpha1'
                                     }
                                   ],

I also tried tools/test-update.sh on develop branch, but it seemed to be still developing (I changed port number from 5434 to 5432), so I tried it manually and confirmed that alter extension pgrouting update to '2.1.0'; worked correctly.

cvvergara commented 8 years ago

@robe2 Regina: I will take into account everything you mention. As soon as I can I will make those changes, because they are important, and maybe @dkastl and @woodbri can decide about Centos, because we haven't made tests for Centos. Probably coordinating this sections with the wiki (which is easier to write than sphinix) and also with the README.md would be better, and also put somewhere the testing matrices like i did in Release Process Checklist Thanks for the comments on this section.

cvvergara commented 8 years ago

@sanak The error for the version is expected because it has the alpha currently but when released it wont have it. About node network probably the error also happens in windows, but right now I am not worrying about that, might be because of machine precision, and nodeNetwork has some issues that have to be solved, so we can take care of that for a future release. (Travis doesn't show that discrepancy, but windows also shows it for a particular configuration) Thanks for the test.

robe2 commented 8 years ago

@cvvergara I just put in a ticket for CentOS / Redhat testing and put @devrimgunduz in the notes, so hopefully he can help us out.

cvvergara commented 8 years ago

@robe2 Thanks.