[
https://issues.apache.org/jira/browse/DERBY-4771?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12966165#action_12966165
]
Dag H. Wanvik commented on DERBY-4771:
--------------------------------------
Last review installment. Just minor issues with the daemon implementation, and
some questions:
A
java/engine/org/apache/derby/iapi/services/daemon/IndexStatisticsDaemon.java
The background mode will try to
affect other operations as little as possible, and errors won't be reported
unless they are severe.
* How is this safe? (errors not reported?) explain! What happens if schedule
throws? Under which condition can work queue be full? Then what hapens?
A
java/engine/org/apache/derby/impl/services/daemon/IndexStatisticsDaemonImpl.java
* LOG_HEADER can be private
* "For instance, it will not set locks on the conglomerates it scans, and if it
needs to take locks it will give up immediately if the locks cannot be
obtained. This isn't true in all cases, which means that the background work
may still interfere with the user activity in the database (besides from using
resources)."
This seems contradictory to me. What are "in all cases" exactly? Not in daemon
mode? Or is the first sentence not true: "if it needs to take locks it will
give up immediately if the locks cannot be obtained"
* Class javadoc IMPROVEMENTS: Note sure I understand the comments about "row
estimate when writing statistics": doesn't this class compute the "real"
numbers? If so, how could they be improved upon?
* daemonDisabled is supposedly guarded by queue monitor. What about access in
updateIndexStatsMinion? Seems to be called from runExplicitly, which does not
syhcronize on "queue"?
* The use of volatile for "daemonStopped" appears to be safe for use even with
Java 1.4 (cf.
http://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#volatile), good.
(I can't see any variable access reordering that could impact its correctness
here),
* Under what conditions can "daemonLCC" be null when "run" is called? (cf test
ca line 4 of that method) I think, if a thread is started up again. Could use a
comment.
* Top of run method: question of philosopy of local use of "final":
long runStart = System.currentTimeMillis();
final ContextService ctxService = ContextService.getFactory();
Both variables are effectively final, but only one is marked as such. Is it
accidental?
* "// TODO: Would be nice to name the transaction."
Interesting, is there a facility to do that in Derby?
* Optional (additional) tracing to std out is just temporary? Or do you want to
keep it in production code as well?
* javadoc for "schedule": "Assume the descriptor will be valid until we get
around to generate the statistics." Is this assured somewhere outside this
class? If not, what would happen if it does not hold?
* the boolean result from "schedule" does not seem to be used. Needed?
If you want this for some future use, the reject due to the table already being
scheduled should probably have another value; it seems less serious than the
queue being full or the daemon beinbg disabled, I think: maybe of of {accepted,
rejected, redundant}.
* runningThread is supposed to be protected by "queue", but setting it to null
in "run"'s "finally" block is not protected, so you could risk a race condition
when determining if thread is running or not on schedule, I think.
"runningThread" need probably be set to false inside the synchronized block of
run where you discover the queue is empty, sinc efater that point, the thread
will invariable terminate, and schedule needs to create a new one.
* } else if (se.getSeverity() >= ExceptionSeverity.DATABASE_SEVERITY) {
// The database or system is going down. Probably handled elsewhere
// but disable daemon anyway.
Do we know this for sure? I thought that usually this would be handled when the
severity bubbled up to handleException on the API level, which in turn calls
TransactionResourceImpl#handleException to do the handling.
* handleUnexpectedErrors#TODO: Do we need a mechanism to disable the daemon if
too many
unexpected errors are raised within a short period of time?"
Probably a good idea, yes.
* run: "// Queue may have been cleared due to a severe failure
// or shutdown."
Where in the code would this happen?
* } catch (ShutdownException se) {
stop(); // Call stop to log activity
ctxMgr.cleanupOnError(se);
Can this happen? I tried to convince myself it could... Answer, yes it can
happen, e.g in Dep man's coreInvalidateFor which does popCompilerContext.
* generateStatistics "TODO: Do we want to retry if we can't get the lock(s)?
If so, maybe add sleep for a while if there are no
other stats to generate?"
Seems you do add sleep now always. Would you want to try another queue task in
meantime, if there is one?
* In invalidateStatements, you always retry when seeing StandardException. Is
that safe? Shouldn't it only be for LOCK_TIMEOUT as in generateStatistics?
* Javadoc for RowCountable#setEstimatedRowCount should probably be updated now:
"For instance if we implement some sort of update statistics command, or just
after a create index a complete scan will have been done of the table." (It is
called from setHeapRowEstimate)
Sorry it took so long to review the patch, much new code for me, edifying
though :)
> Continue investigation of automatic creation/update of index statistics
> -----------------------------------------------------------------------
>
> Key: DERBY-4771
> URL: https://issues.apache.org/jira/browse/DERBY-4771
> Project: Derby
> Issue Type: Task
> Components: SQL, Store
> Affects Versions: 10.8.0.0
> Reporter: Kristian Waagan
> Assignee: Kristian Waagan
> Attachments: autoindexstats.html,
> derby-4771-1a-prototype_code_dump.diff,
> derby-4771-1a-prototype_code_dump.stat,
> derby-4771-1b-prototype_code_dump.diff,
> derby-4771-2a-prototype_lcc_code_dump.diff,
> derby-4771-2b-prototype_lcc_code_dump.diff,
> derby-4771-2c-prototype_lcc_code_dump.diff,
> derby-4771-2d-prototype_lcc_code_dump.diff, DERBY-4771-2e-prototype.rar,
> derby-4771-2e-prototype_lcc_code_dump.diff,
> Derby-4771-2f-AutomaticIndexStatisticsTest_wondows7.rar,
> derby-4771-2f-prototype_lcc_code_dump-WORK-IN-PROGRESS.diff, derby.log,
> error-stacktrace.out, rjall.out, rjall.out, rjall.out, rjall.rar, rjone.out
>
>
> Work was started to improve Derby's handling of index statistics. This issue
> tracks further discussion and work for this task.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
