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
RM_6460_v2.patch
Description: Binary data
