Skip to content

Implement #6413 : Data pages of newly gbak restored databases should marked as "swept" [CORE6164] #8549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Implement #6413 : Data pages of newly gbak restored databases should marked as "swept" [CORE6164] #8549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/burp/BurpTasks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,7 @@ void RestoreRelationTask::initItem(BurpGlobals* tdgbl, Item& item)
m_masterGbl->gbl_dpb_data.begin(),
m_masterGbl->gbl_dpb_data.getCount());

dpb.deleteWithTag(isc_dpb_gbak_attach);
//dpb.deleteWithTag(isc_dpb_gbak_attach);

const UCHAR* dpbBuffer = dpb.getBuffer();
const USHORT dpbLength = dpb.getBufferLength();
Expand Down
42 changes: 39 additions & 3 deletions src/jrd/dpm.epp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2232,7 +2232,14 @@ void DPM_store( thread_db* tdbb, record_param* rpb, PageStack& stack, const Jrd:
memset(data + size, 0, fill);

Ods::pag* page = rpb->getWindow(tdbb).win_buffer;
if (page->pag_flags & dpg_swept)

// When restoring, mark primary data page as swept, pointer page already marked by locate_space()
if (tdbb->getAttachment()->isGbak() && !rpb->rpb_relation->isSystem() && (type == DPM_primary))
{
page->pag_flags |= dpg_swept;
CCH_RELEASE(tdbb, &rpb->getWindow(tdbb));
}
else if (page->pag_flags & dpg_swept)
{
page->pag_flags &= ~dpg_swept;
mark_full(tdbb, rpb);
Expand Down Expand Up @@ -3083,7 +3090,13 @@ static void extend_relation(thread_db* tdbb, jrd_rel* relation, WIN* window, con
UCHAR* bits = (UCHAR*) (ppage->ppg_page + dbb->dbb_dp_per_pp);
PPG_DP_BIT_CLEAR(bits, slot, PPG_DP_ALL_BITS);

if (type != DPM_primary)
if (type == DPM_primary)
{
// When restoring, mark slot as swept, data page will be marked by our caller
if (tdbb->getAttachment()->isGbak() && !relation->isSystem())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I would put this check in a separate inline function like inline bool sweptOnRestore(thread_db* tdbb, jrd_rel* rel).
  2. 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 started gbak -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_creator is also should be passed to parallel worker attachments. To do that, I created isc_dpb_gbak_restore_rel_attach so JProvider::internalAttach can check it along with isc_dpb_gbak_attach and set ATT_creator.

If it's needed, I can provide a patch with these corrections.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I'll consider it.
  2. 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_restore flag (DBB_creating is 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User applications using isc_create_database() can do many things in the attachment, not only restore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

PPG_DP_BIT_SET(bits, slot, ppg_dp_swept);
}
else
PPG_DP_BIT_SET(bits, slot, ppg_dp_secondary);

for (unsigned i = 1; i < cntAlloc; i++)
Expand Down Expand Up @@ -3536,7 +3549,13 @@ static rhd* locate_space(thread_db* tdbb,

PPG_DP_BIT_CLEAR(bits, slot, ppg_dp_empty);
if (type == DPM_primary)
{
PPG_DP_BIT_CLEAR(bits, slot, ppg_dp_secondary);

// When restoring, mark slot as swept, data page will be marked by our caller
if (tdbb->getAttachment()->isGbak() && !relation->isSystem())
PPG_DP_BIT_SET(bits, slot, ppg_dp_swept);
}
else
PPG_DP_BIT_SET(bits, slot, ppg_dp_secondary);

Expand Down Expand Up @@ -3918,11 +3937,28 @@ static void store_big_record(thread_db* tdbb,
header->rhdf_b_page, header->rhdf_b_line);
#endif


bool markPP = false;

// When restoring, mark primary data page as swept, pointer page already marked by locate_space()
if (tdbb->getAttachment()->isGbak() && !rpb->rpb_relation->isSystem() && (type == DPM_primary))
{
page->dpg_header.pag_flags |= dpg_swept;
}
else if (page->dpg_header.pag_flags & dpg_swept)
{
page->dpg_header.pag_flags &= ~dpg_swept;
markPP = true;
}

if (!(page->dpg_header.pag_flags & dpg_large))
{
page->dpg_header.pag_flags |= dpg_large;
mark_full(tdbb, rpb);
markPP = true;
}

if (markPP)
mark_full(tdbb, rpb);
else
CCH_RELEASE(tdbb, &rpb->getWindow(tdbb));
}
Loading