Re: Next release

Hi

On Fri, Aug 25, 2017 at 2:04 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

>
>
> On Fri, Aug 25, 2017 at 9:28 AM, Harshal Dhumal <
> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>
>> One more thing that this will only work for future pgAdmin4 versions
>>
>
>
> Yeah :-(
>
>
>>
>> --
>> *Harshal Dhumal*
>> *Sr. Software Engineer*
>>
>> EnterpriseDB India: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>> On Fri, Aug 25, 2017 at 1:48 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>>
>>>
>>> On Fri, Aug 25, 2017 at 9:15 AM, Surinder Kumar <
>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>
>>>> ​Hi​
>>>>
>>>> On Fri, Aug 25, 2017 at 12:21 PM, Ashesh Vashi <
>>>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> On Fri, Aug 25, 2017 at 12:16 PM, Surinder Kumar <
>>>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Hi
>>>>>> On Fri, Aug 25, 2017 at 1:03 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Aug 24, 2017 at 8:28 PM, Harshal Dhumal <
>>>>>>> harshal(dot)dhumal(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> *Harshal Dhumal*
>>>>>>>> *Sr. Software Engineer*
>>>>>>>>
>>>>>>>> EnterpriseDB India: http://www.enterprisedb.com
>>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>>
>>>>>>>> On Thu, Aug 24, 2017 at 9:44 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Thu, Aug 24, 2017 at 10:36 AM, Surinder Kumar <
>>>>>>>>> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>>>>>>>>>
>>>>>>>>>> Hi Dave,
>>>>>>>>>>
>>>>>>>>>> On Thu, Aug 24, 2017 at 2:28 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Anyone object to doing a release on 14th September, wrapping the
>>>>>>>>>>> code on Monday 11th? This seems like the best option for our QA folks who
>>>>>>>>>>> will be off for EID somewhen in the two weeks before.
>>>>>>>>>>>
>>>>>>>>>>> Assuming not, should this be 1.7 or 2.0?
>>>>>>>>>>>
>>>>>>>>>>> If we go with 2.0, it'll be for "safety" given the proposed
>>>>>>>>>>> changes to path management to allow both server and desktop modes to work
>>>>>>>>>>> out of the box on Linux.
>>>>>>>>>>>
>>>>>>>>>>> If we do that, we also need to ensure that any changes to the
>>>>>>>>>>> config database are backwards compatible, as a 2.0 release would be a
>>>>>>>>>>> side-by-side installation. Surinder; was it you that had looked into that?
>>>>>>>>>>>
>>>>>>>>>> ​I had looked into this and here are my findings:
>>>>>>>>>> 1. If we are using newer version of pgAdmin and the go back to
>>>>>>>>>> older version of pgAdmin, then on running `python pgAdmin4.py`. the
>>>>>>>>>> flask-migrate(Alembic) try to perform downgrade by one step only(ie. it can
>>>>>>>>>> switch back to one migration only when we run `python pgAdmin4.py`). But
>>>>>>>>>> we have multiple database revisions to be migrated. So migration fails here.
>>>>>>>>>>
>>>>>>>>>> 2. When Alebmic downgrade is performed by one step, it looks for
>>>>>>>>>> downgrade function in that specific database revision, but in our code we
>>>>>>>>>> didn't written downgrade function. But if we have written downgrade
>>>>>>>>>> statement, still there is an issue:
>>>>>>>>>> ie. If we add a new column to a table xyz using ALTER statement
>>>>>>>>>> like:
>>>>>>>>>>
>>>>>>>>>> ​```
>>>>>>>>>> ​
>>>>>>>>>> def upgrade():
>>>>>>>>>> ​ ​
>>>>>>>>>> verison = get_version()
>>>>>>>>>>
>>>>>>>>>> ​ ​
>>>>>>>>>> db.engine.execute(
>>>>>>>>>> ​ ​
>>>>>>>>>> 'ALTER TABLE server ADD COLUMN hostaddr TEXT(1024)'
>>>>>>>>>> ​ ​
>>>>>>>>>> )
>>>>>>>>>>
>>>>>>>>>> def downgrade():
>>>>>>>>>> ​ ​
>>>>>>>>>> pass
>>>>>>>>>> ​```​
>>>>>>>>>> then on downgrade it executes `downgrade` method, so downgrade
>>>>>>>>>> should have code like
>>>>>>>>>> `ALTER TABLE server DROP COLUMN hostaddr `
>>>>>>>>>> but in sqlite DROP COLUMN statements don't work.
>>>>>>>>>> So, this is a an issue with Sqlite database. However, an
>>>>>>>>>> alternative way is also given. Here is link
>>>>>>>>>> <https://stackoverflow.com/questions/5938048/delete-column-from-sqlite-table>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Still, I didn't find any other solution on upgrading/downgrading
>>>>>>>>>> database revisions without errors.
>>>>>>>>>> It is an issue with Flask-Migrate(Alembic) plugin.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Urgh. So I guess the other option is that we version the DB
>>>>>>>>> filename as well. The downside of that is that users will want to migrate
>>>>>>>>> their settings - which may be awkward as we'll have no real way of knowing
>>>>>>>>> where they are.
>>>>>>>>>
>>>>>>>>> Thoughts?
>>>>>>>>>
>>>>>>>>> Or should we write our own custom backword migrations? For eg.
>>>>>>>> dropping column can be achieved by creating another table excluding the
>>>>>>>> columns which we want to drop then copy data to new table and then drop old
>>>>>>>> table and rename new table to old name. And also sqlite database schema
>>>>>>>> which we have in pgAdmin4 is small so writing and maintaining custom
>>>>>>>> migration won be that hard.
>>>>>>>>
>>>>>>>
>>>>>>> The problem is that we don't want to migrate backwards; we want both
>>>>>>> versions to be able to run with the same database (for example, because you
>>>>>>> might have multiple versions installed with the EDB PG installer as I do on
>>>>>>> my laptop).
>>>>>>>
>>>>>>> Previously, we always made sure our changes were backwards
>>>>>>> compatible (e.g. by only adding new columns, never removing or renaming
>>>>>>> them), and our home-grown migration code only cared about upgrading the
>>>>>>> database to the current version; it wouldn't complain if the database was
>>>>>>> of a newer version.
>>>>>>>
>>>>>> ​The code which is responsible to run database migration is
>>>>>> `db_upgrade(app)` in `pgadmin/__init__.py​` it executes when python server
>>>>>> runs `python pgAdmin4.py`, It fails with older version of pgAdmin4(say 1.5)
>>>>>> because it cannot find db revision file (revision id stored in table
>>>>>> 'alembic_version') in `web/migrations` folder of latest pgAdmin4-1.5
>>>>>>
>>>>>> But If we catch this exception like:
>>>>>> ```
>>>>>> import alembic
>>>>>> try:
>>>>>> db_upgrade(app)
>>>>>> except alembic.util.exc.CommandError as e: # Handle migration
>>>>>> error, I expect this exception will be raised in older version of code.
>>>>>> app.logger.info('Failed to run migrations: %s' % str(e))
>>>>>> ```
>>>>>>
>>>>>> It will fail to run migrations but exception will be handled and
>>>>>> python app server will be started successfully and pgAdmin4 will run with
>>>>>> newer database.
>>>>>> Or, we should check whether the migration which is about to run
>>>>>> against the revision id(stored in table alembic_version) exists or not in
>>>>>> `web/migrations`.
>>>>>> If it exists then run migration otherwise don't run.
>>>>>>
>>>>>> This way the same database will work for pgAdmin4-1.5 and pgAdmin4-1.6
>>>>>> But the only problem is that we didn't caught exception
>>>>>> `alembic.util.exc.CommandError` in older versions of pgAdmin4.
>>>>>>
>>>>> As per my understanding, that's not safe, as we may not be able to
>>>>> catch some other genuine issues.
>>>>>
>>>> ​Yes, that's the possibility.
>>>> I discussed this with Harshal and discussed about what other possible
>>>> workarounds can work.
>>>> So, here is the one:
>>>>
>>>> We will store the current pgAdmin4 version in sqlite table if we are
>>>> not storing. Also, current pgAdmin4 version is assigned to config variable
>>>> `APP_VERSION` in config.py
>>>>
>>>> And when running older pgAdmin4 version with new database,
>>>> we will compare `if config.
>>>> APP_VERSION
>>>> ​ < PGADMIN4_VERSION`​, then skip migration, otherwise run.
>>>> *For example:*
>>>>
>>>> if config.
>>>> APP_VERSION
>>>> ​ >= PGADMIN4_VERSION: # Run migration only when
>>>>
>>>> ​ db_upgrade(app) # run migration​
>>>> # Now update PGADMIN4_VERSION in sqlite to latest one
>>>>
>>>
>>> That sounds like a reasonable approach, though for developers I think
>>> the pgAdmin version wouldn't necessarily work - we'd need a schema version
>>> like we used to have.
>>>
>> ​Yes, that's true.
We will use config variable `
SETTINGS_SCHEMA_VERSION
​`. We had previously used this variable and current schema version is
stored in `version` table.
I will send a patch for the same.

>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>