Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lojqQ-0003FN-W2 for pgadmin-hackers@arkaria.postgresql.org; Thu, 03 Jun 2021 09:40:55 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1lojqP-0001sx-R9 for pgadmin-hackers@arkaria.postgresql.org; Thu, 03 Jun 2021 09:40:53 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lojqP-0001sp-Dh for pgadmin-hackers@lists.postgresql.org; Thu, 03 Jun 2021 09:40:53 +0000 Received: from mail-yb1-xb34.google.com ([2607:f8b0:4864:20::b34]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1lojqM-0001jp-N1 for pgadmin-hackers@postgresql.org; Thu, 03 Jun 2021 09:40:52 +0000 Received: by mail-yb1-xb34.google.com with SMTP id g38so7911841ybi.12 for ; Thu, 03 Jun 2021 02:40:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=sarxQ+2fR8mpv0S40gVASaUk300AYsL5EOCvuBhwQyM=; b=UBzwjDX9as6yM1BLtVGOsD3OkAUVikmOAoxCZ9kFsFBAj/IeBHKOFjdoBQAsaqAX36 hBroaBu4iDqopeTYLkBttnzTYradnO4UZPXgGZJkRk2yaY+tCB44fRLuon1l1MdTOZ/W 2LYEZNGBGIUs/Xyh7sXlcVje323e/MnKZkw1LyS06bS/KDUR0Egz9MhNS+x0Pp/gdnkO koFbckRwYA7FKadp+0CnyvmcAyeb0WOUG7jP4AbVmS43gqHFqsbHylIR94QmbvTaGfYR VwYJkzZj/LDr3bzS8EeMwD49AbE9Hn8Un9HaIwPmel8+mfyXJX37g6LjQyxVgi18xORv P2CQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=sarxQ+2fR8mpv0S40gVASaUk300AYsL5EOCvuBhwQyM=; b=fCQJTdO7ZGylJnTGno11GLI6TlMUR/DJbjliEeL5UULrCkPQ0B8mewrU32vKWdXHA/ grBY29wA0x47cCmBGNz7wKsR6MVzCwMabiuq58hH9je21o/cn1Z5da9a1nP7ofG3t1h2 Kv6ZrntUYopkPAAGvmuRrIRBIwItlmnklTvtINZXu/FxPzqmRy3Mdtzmv2PmRz0obfwl jutyidstXri9q9+codSORC9QIGaB/OfS5kCw5R1b1WF3zKPlcJR9I/4o98PCLWla2FDN MFVCAeCAnNgG+YZJw4sC/BNhiUxDP1PRz8TGl0/vv5XPSBdYNJIYC6HFZdmdx7Y323Ah +g0Q== X-Gm-Message-State: AOAM533VaL0yYZIfY6aWkiM10tH1HqJzUvAInRU80xoIwqmuqzLBBVYz v89Pb0Yr7nYAoFq0mDp50Hq9PdfC8ciz1Pt0Q8D/geOgacOKjQ24e2a3WZBxm97QoeO2NPKjq8/ 8ePx9rIoWcIG3ldHsMynFLCTA3nG4njoG58DWl+5WQGQk5+hRjgr93rhNQwMiG1GnWF8mBRGI5l ktajs3btlBb9czmR+KOso6kTKLWDKAH5l/qYUk2tG/jAPCzvZ/d0Ftdsana1k+6fA= X-Google-Smtp-Source: ABdhPJwKJxjnp6DsY2NXEHK+o988Z63UI00ZEd0dhG8oHu+8TBtQNuljXFpQ6gF8FlDN5vfmDUJBvWZB9GezkRsYhMY= X-Received: by 2002:a25:8b08:: with SMTP id i8mr48893460ybl.370.1622713249806; Thu, 03 Jun 2021 02:40:49 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Nikhil Mohite Date: Thu, 3 Jun 2021 15:10:38 +0530 Message-ID: Subject: Re: [pgAdmin][RM-6460]: Need a mechanism to detect a corrupt/broken config DB file. To: Dave Page Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000d3d59a05c3d95e1f" X-CLOUD-SEC-AV-Info: enterprisedb,google_mail,monitor X-CLOUD-SEC-AV-Sent: true X-Gm-Spam: 0 X-Gm-Phishy: 0 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000d3d59a05c3d95e1f Content-Type: text/plain; charset="UTF-8" Hi Dave, On Thu, Jun 3, 2021 at 2:49 PM Dave Page 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 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 --000000000000d3d59a05c3d95e1f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Dave,

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

On Thu, Jun 3, 2021 at 10:01 AM Nikhil Moh= ite <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 pa= tch for=C2=A0RM-6460:=C2=A0 Need a mechanism to detect a corrupt/broken = config DB file.

1. Added checks if all tables adde= d 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 fi= le.=C2=A0
(User will get notification on UI=C2=A0for the location= =C2=A0of the backup file and newly created.)
3. If the user delet= ed 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 c= heck in point 1?
Yes, but if the user de= letes any table while pgAdmin is running then it will fail when the user tr= ies to run pgAdmin next time.

=
Right - but how is the end result of that different from a failed migr= ation that=C2=A0left a table missing? Either way, the end result is the sam= e, and should be handled the same way; i.e. backup/recreate the DB, then wa= rn the user as soon as the UI is up.
Yes - It should be the same in both conditions, I will send an updated p= atch for this.
=C2=A0

<= /div>
I believe the process is simple (and maybe this is what is done a= nd 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, backu= p the file, create a new one from scratch, and warn the user.
One thing I did notice when skimming the patch - we have this i= n the code:

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0raise RuntimeError(
+ =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0'Specified SQLite d= atabase file is not valid.')

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

"The con= figuration database file is not valid."

Thank= s!

=C2=A0
(If we remo= ve the table from the model it will not check particular table is present i= n DB or not. )
=C2=A0
--
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter:= @pgsnake

EDB: https://www.enterprisedb.com

Regards,
Nikhil Mohite.=C2=A0


--
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake<= br>
EDB: http= s://www.enterprisedb.com

Regards,
Nikhil Mohite=C2=A0
--000000000000d3d59a05c3d95e1f--