db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dag H. Wanvik (JIRA)" <j...@apache.org>
Subject [jira] Commented: (DERBY-4771) Continue investigation of automatic creation/update of index statistics
Date Thu, 02 Dec 2010 16:33:10 GMT

    [ 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.


Mime
View raw message