Thanks, the patch applied. On Thu, Jun 3, 2021 at 3:52 PM Nikhil Mohite <nikhil.moh...@enterprisedb.com> wrote:
> Hi Team, > > PFA updated patch (v2) > On Thu, Jun 3, 2021 at 3:10 PM Nikhil Mohite < > nikhil.moh...@enterprisedb.com> wrote: > >> 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 >> > Regards, > Nikhil Mohite > -- *Thanks & Regards* *Akshay Joshi* *pgAdmin Hacker | Principal Software Architect* *EDB Postgres <http://edbpostgres.com>* *Mobile: +91 976-788-8246*
