Implement #6413 : Data pages of newly gbak restored databases should marked as "swept" [CORE6164] #8549
Conversation
…marked as "swept" [CORE6164]
|
Why exception for system relations? |
There is no promise that system tables contains no backversions after restore. |
|
Vlad, thank you!!! |
Does this flag prevent garbage collection as well? System tables are read extremely intensively so hardly any garbage in them can left uncollected. |
Read the code.
Do you give guarantee ? Run gbak -r and gstat -r -s after it. |
src/jrd/dpm.epp
Outdated
| if (type == DPM_primary) | ||
| { | ||
| // When restoring, mark slot as swept, data page will be marked by our caller | ||
| if (tdbb->getAttachment()->isGbak() && !relation->isSystem()) |
There was a problem hiding this comment.
- I would put this check in a separate inline function like
inline bool sweptOnRestore(thread_db* tdbb, jrd_rel* rel). isGbak()is not 100% safe to use. I remember I tested a case where I created some DB trigger which inserts/updates data. And when I startedgbak -b, the normal swept flag logic wasn't working during execution of the trigger. So we need to distinguish between backup and restore.isRWGbak()is more suitable here but it's not going to work right away.ATT_creatoris also should be passed to parallel worker attachments. To do that, I createdisc_dpb_gbak_restore_rel_attachsoJProvider::internalAttachcan check it along withisc_dpb_gbak_attachand setATT_creator.
If it's needed, I can provide a patch with these corrections.
There was a problem hiding this comment.
- I'll consider it.
- Good advise, thanks. I think we can handle "backup vs restore" issue without a need in new DPB tag: let creator attachment mark dbb with new
DBB_restoreflag (DBB_creatingis already exists and serve another purposes) and clear this flag on disconnect. The only risk here is if user creates attachment while database is restored and changes some data, but this is not supported anyway.
There was a problem hiding this comment.
User applications using isc_create_database() can do many things in the attachment, not only restore.
There was a problem hiding this comment.
2. The only risk here is if user creates attachment while database is restored
This is possible only when restore is parallel due to DB multi shutdown mode, right? Maybe, when DBB_restore is set, only attachments with isc_dpb_gbak_attach should be allowed.
There was a problem hiding this comment.
- The only risk here is if user creates attachment while database is restored
This is possible only when restore is parallel due to DB multi shutdown mode, right?
Yes
Maybe, when
DBB_restoreis set, only attachments withisc_dpb_gbak_attachshould be allowed.
Done. But it is a bit limited in CS\SC case - another process could bypass this check. Anyway, I think it is better than nothing. Later we can implement more robust protection, if required.
There was a problem hiding this comment.
But it is a bit limited in CS\SC case - another process could bypass this check
BTW, the parallel restore in CS is still setting the swept flag normally after you added and used m_restoring?
There was a problem hiding this comment.
Now it should work in all server modes.
BTW, I'm not sure we should prevent all kind of user attachments during restore.
For example, it make impossible to correctly stop restore process in some cases.
Without this restriction one could stop restore by full shutdown restoring database or kill gbak attachment using monitoring tables.
There was a problem hiding this comment.
BTW, I'm not sure we should prevent all kind of user attachments during restore.
For example, it make impossible to correctly stop restore process in some cases.
From this point of view, did you consider the solution I proposed in the first comment ? Also, I need to mention another difference between our implementations which can be important. In my variant extend_relation is responsible for setting the swept flag on DPs during restore instead of DPM_store/store_big_record. Due to this, even changing data in user attachments becomes safe, if I understand correctly. And when restore ends, changed DPs will just end up without the swept flag which is totally fine for this unusual case, I guess.
There was a problem hiding this comment.
BTW, I'm not sure we should prevent all kind of user attachments during restore.
For example, it make impossible to correctly stop restore process in some cases.From this point of view, did you consider the solution I proposed in the first comment ?
Yes, I did. I prefer to not change gbak and not introduce new DPB items just for this task. Note, with your solution and using old gbak with new server, state of the restored database will depend on command line (if services was used or not). I don't think it is consistent and prefer not to explain users why restored database was swept in one case and not swept in another.
Also, I need to mention another difference between our implementations which can be important. In my variant
extend_relationis responsible for setting the swept flag on DPs during restore instead ofDPM_store/store_big_record.
I suppose, you set swept flag on newly allocated DP immediately after formatting and make DPM_store and store_big_record to not clear it during restore, correct ?
I see no principal difference here, while your way need a bit less code changes, agree.
Due to this, even changing data in user attachments becomes safe, if I understand correctly.
Only if they didn't consider database as "restoring". And this is same for my way too.
The really important difference is that I made "restoring" an global database state, while you made it a local per-connection attribute.
And when restore ends, changed DPs will just end up without the swept flag which is totally fine for this unusual case, I guess.
Yes. But anyway I consider ability for user to change data during restore as a problem that better to avoid.
There was a problem hiding this comment.
I suppose, you set swept flag on newly allocated DP immediately after formatting and make
DPM_storeandstore_big_recordto not clear it during restore, correct ?
Correct. The difference is when DPM_store is called several times for the same DP, it has no chance to set the swept flag back if it was recently cleared by a concurrent user attachment. But if we don't allow to change data - it doesn't matter, and both approaches are fine.
… the restoring database, extract commonly used function swept_at_restore() as Ilya Eremin (@ilya071294) suggested.
Co-authored-by: Adriano dos Santos Fernandes <[email protected]>
No description provided.