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

From: Nikhil Mohite <[email-masked]>
Date: Thu, 3 Jun 2021 12:09:05 +0530

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.

-- 
*Thanks & Regards,*
*Nikhil Mohite*
*Software Engineer.*
*EDB Postgres* <https://www.enterprisedb.com/>
*Mob.No: +91-7798364578.*

---

From: Dave Page <[email-masked]>
Date: Thu, 3 Jun 2021 09:17:41 +0100

Hi

On Thu, Jun 3, 2021 at 7:39 AM Nikhil Mohite <[email-masked]>
wrote:

> <https://redmine.postgresql.org/issues/6460>:  Need a mechanism to detect
> a corrupt/broken config DB file.
>
> 2. If migrations fail it will backup older file and try migrations with
> the newly created file.
> 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?

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

EDB: https://www.enterprisedb.com

---

From: Nikhil Mohite <[email-masked]>
Date: Thu, 3 Jun 2021 14:31:32 +0530

Hi Dave,

On Thu, Jun 3, 2021 at 1:47 PM Dave Page <[email-masked]> wrote:

> Hi
>
> On Thu, Jun 3, 2021 at 7:39 AM Nikhil Mohite <
> [email-masked]> wrote:
>
>> Hi Hackers,
>>
>> <https://redmine.postgresql.org/issues/6460>:  Need a mechanism to
>> detect a corrupt/broken config DB file.
>>
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.
(If we remove the table from the model it will not check particular table
is present in DB or not. )

> Regards,
Nikhil Mohite.

---

From: Dave Page <[email-masked]>
Date: Thu, 3 Jun 2021 10:19:00 +0100

Hi

On Thu, Jun 3, 2021 at 10:01 AM Nikhil Mohite <
[email-masked]> wrote:

>>> 1. Added checks if all tables added in the model are present in SQLite
>>> DB or not.
>>> (User will get notification on UI for the location of the backup file
>>> and newly created.)

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.

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!

---

From: Nikhil Mohite <[email-masked]>
Date: Thu, 3 Jun 2021 15:10:38 +0530

Hi Dave,

On Thu, Jun 3, 2021 at 2:49 PM Dave Page <[email-masked]> wrote:

> 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):
>
> - If there's a problem, backup the file, create a new one from scratch,
> and warn the user.
>
Nikhil Mohite

---

From: Nikhil Mohite <[email-masked]>
Date: Thu, 3 Jun 2021 15:51:50 +0530

Hi Team,

PFA updated patch (v2)
On Thu, Jun 3, 2021 at 3:10 PM Nikhil Mohite <[email-masked]>
wrote:

>>>>> 2. If migrations fail it will backup older file and try migrations
>>>>> with the newly created file.
>>>>> 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.
>>>>>
>>>>

>>> (If we remove the table from the model it will not check particular
>>> table is present in DB or not. )
>>>
>>>>
>>>> --
>>>> Dave Page
Regards,
Nikhil Mohite

---

From: Akshay Joshi <[email-masked]>
Date: Tue, 8 Jun 2021 19:57:37 +0530

Thanks, the patch applied.

On Thu, Jun 3, 2021 at 3:52 PM Nikhil Mohite <[email-masked]>
wrote:

> Hi Team,
>
> PFA updated patch (v2)
> On Thu, Jun 3, 2021 at 3:10 PM Nikhil Mohite <
>>>>>> 1. Added checks if all tables added in the model are present in
>>>>>> SQLite DB or not.

-- 
*Thanks & Regards*
*Akshay Joshi*
*pgAdmin Hacker | Principal Software Architect*
*EDB Postgres <http://edbpostgres.com>*

*Mobile: +91 976-788-8246*