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

Hi Team,

PFA updated patch (v2)
On Thu, Jun 3, 2021 at 3:10 PM Nikhil Mohite <nikhil(dot)mohite(at)enterprisedb(dot)com>
wrote:

> Hi Dave,
>
> On Thu, Jun 3, 2021 at 2:49 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>> Hi
>>
>> On Thu, Jun 3, 2021 at 10:01 AM Nikhil Mohite <
>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Dave,
>>>
>>> On Thu, Jun 3, 2021 at 1:47 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>
>>>> Hi
>>>>
>>>> On Thu, Jun 3, 2021 at 7:39 AM Nikhil Mohite <
>>>> nikhil(dot)mohite(at)enterprisedb(dot)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