Hi Dave, On Thu, Jun 3, 2021 at 2:49 PM Dave Page <dp...@pgadmin.org> wrote:
> Hi > > On Thu, Jun 3, 2021 at 10:01 AM Nikhil Mohite < > nikhil.moh...@enterprisedb.com> wrote: > >> Hi Dave, >> >> On Thu, Jun 3, 2021 at 1:47 PM Dave Page <dp...@pgadmin.org> wrote: >> >>> Hi >>> >>> On Thu, Jun 3, 2021 at 7:39 AM Nikhil Mohite < >>> nikhil.moh...@enterprisedb.com> wrote: >>> >>>> Hi Hackers, >>>> >>>> Please find the attached patch for RM-6460 >>>> <https://redmine.postgresql.org/issues/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
