Re: [pgAdmin][RM-6460]: Need a mechanism to detect a corrupt/broken config DB file.

Hi Team,

PFA updated patch (v2)

On Thu, Jun 3, 2021 at 3:10 PM Nikhil Mohite <nikhil.mohite@enterprisedb.com> wrote:

Hi Dave,

On Thu, Jun 3, 2021 at 2:49 PM Dave Page <dpage@pgadmin.org> wrote:

Hi

On Thu, Jun 3, 2021 at 10:01 AM Nikhil Mohite <nikhil.mohite@enterprisedb.com> wrote:

Hi Dave,

On Thu, Jun 3, 2021 at 1:47 PM Dave Page <dpage@pgadmin.org> wrote:

Hi

On Thu, Jun 3, 2021 at 7:39 AM Nikhil Mohite <nikhil.mohite@enterprisedb.com> wrote:

Hi Hackers,

Please find the attached patch for RM-6460:  Need a mechanism to detect a corrupt/broken config DB file.

1. Added checks if all tables added in the model are present in SQLite DB or not.

2. If migrations fail it will backup older file and try migrations with the newly created file. 

(User will get notification on UI for the location of the backup file and newly created.)

3. If the user deleted any table from SQLite DB pgAdmin will not run on the next restart and it will add the missing table list in the logs.

Surely if any tables have been deleted, it'll fail the check in point 1?

Yes, but if the user deletes any table while pgAdmin is running then it will fail when the user tries to run pgAdmin next time.

Right - but how is the end result of that different from a failed migration that left a table missing? Either way, the end result is the same, and should be handled the same way; i.e. backup/recreate the DB, then warn the user as soon as the UI is up.

Yes - It should be the same in both conditions, I will send an updated patch for this. 

 

I believe the process is simple (and maybe this is what is done and this is just a language issue - I haven't read the patch in detail):

- On startup, attempt to create the database/run any migrations as appropriate.

- Check that all tables exist, and the schema version is correct.

- If there's a problem, backup the file, create a new one from scratch, and warn the user.

One thing I did notice when skimming the patch - we have this in the code:

+                raise RuntimeError(

+                    'Specified SQLite database file is not valid.')

The *user* didn't specify a SQLite database file (nor do they care that it's SQLite at this point). That (and any other similar messages) should probably be rephrased to say something like:

"The configuration database file is not valid." 

Thanks!

 

(If we remove the table from the model it will not check particular table is present in DB or not. )

 

--

Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: https://www.enterprisedb.com

Regards,

Nikhil Mohite. 

--

Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: https://www.enterprisedb.com

Regards,

Nikhil Mohite 

Regards,

Nikhil Mohite