Opened 7 years ago

Closed 7 years ago

#2877 closed defect (fixed)

slow updates to SQLite3 data source

Reported by: jinmei Owned by: vorner
Priority: medium Milestone: Sprint-20130423
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 4.21 Internal?: no

Description

See https://lists.isc.org/pipermail/bind10-dev/2013-March/004523.html

"2.5 minutes for 70 records of update" looks too slow. I don't think
it's because of the machine power or HDD speed. There should be some
significant bottleneck either in our own code or in a way we use
SQLite3.

I don't think we see such slow updates in test environments, so it may
also be specific to very large zones. I suggest we first try to
reproduce it ourselves, then figure out the cause of it, and, if it's
still manageable, fix it.

Subtickets

Change History (19)

comment:1 Changed 7 years ago by jinmei

And see
https://lists.isc.org/pipermail/bind10-dev/2013-April/004553.html
We now know the cause and a possible fix.

The proposed patch attached to the bind10-dev email message is almost
complete. Assuming the solution is reasonable, we'll only have to
wrap it up with documentation and other cleanups (I believe no
additional tests are needed), get it reviewed and merged.

At this opportunity, I also suggest adding some detailed logs for
database update operations. See the attached patch in
https://lists.isc.org/pipermail/bind10-dev/2013-March/004544.html
Such detailed logs will be helpful when we need to chase other issues
like this one in future.

comment:2 Changed 7 years ago by jinmei

  • Priority changed from medium to very high

comment:3 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 8

comment:4 Changed 7 years ago by muks

  • Estimated Difficulty changed from 8 to 3
  • Milestone changed from Next-Sprint-Proposed to Sprint-20130423

comment:5 Changed 7 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:6 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

It should be ready for review. I applied the patches from the mailing list and added some documentation and descriptions for the log messages.

We might want to have a changelog message (I'm listing jinmei as the author, since he wrote the real patches and found the cause):

[bug]	jinmei
IXFR and DDNS Updates to the sqlite3 database are no longer so slow on large
zones as they were before.

I believe DDNS would have been hit by this too, so it is listed there.

comment:7 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to vorner

comment:8 Changed 7 years ago by jinmei

  • Owner changed from vorner to jinmei

oops, assigned to the wrong person...

comment:9 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by jinmei

Replying to vorner:

It should be ready for review. I applied the patches from the mailing list and added some documentation and descriptions for the log messages.

We might want to have a changelog message (I'm listing jinmei as the author, since he wrote the real patches and found the cause):

[bug]	jinmei
IXFR and DDNS Updates to the sqlite3 database are no longer so slow on large
zones as they were before.

I believe DDNS would have been hit by this too, so it is listed there.

I'd describe some more details. Suggestion:

599.?	[bug]*		jinmei(, vorner?)
	The "delete record" interface of the database based data source
	was extended do that the parameter includes reversed name in
	addition to the actual name.  This may help the underlying
	accessor implementation if reversed names are more convenient
	for the delete operation.  This was the case for the SQLite3
	accessor implementation, and it now performs delete operations
	much faster.  At a higher level, this means IXFR and DDNS Updates
	to the sqlite3 database are no longer so slow on large zones as
	they were before.
	(Trac #2877, git TBD)

as for the "credit", I think any developer that made non trivial
contribution to the task (in this case, it's you) deserves it, but I'd
leave it to you.

Regarding the branch:

  • I think we need some more explanation of why we provide DEL_RNAME in documentation. I've committed my proposed text.
  • I wonder whether we should now make DeleteRecordParams non-NSEC3 records only, just like we separate AddRecordColumns and AddNSEC3RecordColumns. It's a tradeoff between reducing the amount of code and disadvantages of overloading (conceptual confusion, need for additional tweak in the accessor side), but at this point I guess the latter outweighs the former. If we keep overloading, we'll at least need what's in RNAME for NSEC3 and what's accessor should do with it. I'd also guess we'd rather specify an empty string in RNAME for the NSEC3 case.

comment:10 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:11 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

I'd describe some more details. Suggestion:

OK, I'll use it (with fixing the typo: s/to the actual/so the actual/).

  • I think we need some more explanation of why we provide DEL_RNAME in documentation. I've committed my proposed text.

Looks good.

  • I wonder whether we should now make DeleteRecordParams non-NSEC3 records only, just like we separate AddRecordColumns and AddNSEC3RecordColumns. It's a tradeoff between reducing the amount of code and disadvantages of overloading (conceptual confusion, need for additional tweak in the accessor side), but at this point I guess the latter outweighs the former. If we keep overloading, we'll at least need what's in RNAME for NSEC3 and what's accessor should do with it. I'd also guess we'd rather specify an empty string in RNAME for the NSEC3 case.

The separation seems to make sense to me. But I think we might want to do it in a separate ticket. Would you agree with merging it this way, to solve the slow updates, and create a ticket for the cleanup?

I updated the documentation for the RNAME column.

comment:12 in reply to: ↑ 11 Changed 7 years ago by jinmei

Replying to vorner:

  • I wonder whether we should now make DeleteRecordParams non-NSEC3 records only, just like we separate AddRecordColumns and AddNSEC3RecordColumns. It's a tradeoff between reducing the amount of code and disadvantages of overloading (conceptual confusion, need for additional tweak in the accessor side), but at this point I guess the latter outweighs the former. If we keep overloading, we'll at least need what's in RNAME for NSEC3 and what's accessor should do with it. I'd also guess we'd rather specify an empty string in RNAME for the NSEC3 case.

The separation seems to make sense to me. But I think we might want to do it in a separate ticket. Would you agree with merging it this way, to solve the slow updates, and create a ticket for the cleanup?

If we both think it makes sense, I'd be inclined to complete it within
this ticket, as I believe it's basically an easy addition while I
suspect it would be quite possible that the separate ticket is
considered less important than others and not adopted in near feature
or ever.

So my specific suggestion is to merge the current branch to master,
keep this ticket open, and complete the cleanup work within this task.

comment:13 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:14 Changed 7 years ago by vorner

  • Priority changed from very high to medium

OK then.

I merged the branch and I'm lowering the priority, as it is no longer urgent.

comment:15 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

It turned out to be simpler than I expected. So, ready for review again.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 7 years ago by jinmei

Replying to vorner:

It turned out to be simpler than I expected. So, ready for review again.

The changes basically look okay. I made a few minor cleanups.

One additional comment: in database_unittest, we might use upper case
characters for the template parameter:

    template<size_t PARAM_COUNT>
    void deleteRecord(Domains& domains, const string (&params)[PARAM_COUNT]) {

so that it's clearer in the source code that param_count is not a
normal variable.

But this is not per the guideline but rather a matter of taste, so I'd
leave it to you.

comment:17 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:18 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 1.2

comment:19 in reply to: ↑ 16 Changed 7 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 1.2 to 4.21

Hello

Replying to jinmei:

The changes basically look okay. I made a few minor cleanups.

One additional comment: in database_unittest, we might use upper case
characters for the template parameter:

    template<size_t PARAM_COUNT>
    void deleteRecord(Domains& domains, const string (&params)[PARAM_COUNT]) {

I don't think this is consistent with the rest of the code. If you look at templates (which are usually class names), they are usual CamelCase for class names (or single-character identifiers). So it seems more consistent to leave it as ordinary variable, instead of using a constant.

The branch is merged.

Note: See TracTickets for help on using tickets.