Bug 116193 - p2fs performance improvements
Summary: p2fs performance improvements
Alias: None
Product: Red Hat Enterprise CMS
Classification: Retired
Component: other (Show other bugs)
(Show other bugs)
Version: nightly
Hardware: All Linux
Target Milestone: ---
Assignee: Richard Li
QA Contact: Jon Orris
Depends On:
Blocks: 108447 113496
TreeView+ depends on / blocked
Reported: 2004-02-18 21:11 UTC by Richard Li
Modified: 2007-04-18 17:03 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2004-03-05 20:26:06 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)

Description Richard Li 2004-02-18 21:11:38 UTC
From Carsten:

After finding that the query executed by QueueManager.flagBlock() was
responsible for a large number of Oracle buffer gets on a test system
where P2FS pollDelay was set to 5s, I have changed processQueueItems()
to only use one select query. This not only makes life easier for the
database, but also simplifies the P2FS code. I think the update query in
the old code was a left over from the time when we had no host_id in the
publish_to_fs_queue table. With each JVM only processing its own
entries, it is no longer necessary to first flag and then process a
block. Also, the old code executed its select query twice when there
were items to be processed.

The changelist is 40509.

WRT to DP: Alan tuned the update query in changelist 37999. But he did
it in a way that only works for Oracle and drops support for the
queueEntryRetryDelay parameter and for negative values of the
maximumFailCount parameter. While my change gets rid of the update
query completely.

Comment 1 Richard Li 2004-02-19 20:31:50 UTC
It turns out merging this is actually non-trivial. The new getBlock
query works only on Oracle (due to its use of native Oracle
date-isms). I wrote a revised PG query using EXTRACT, which works,
except that the PDL parser doesn't actually parse the query correctly,
because EXTRACT uses the 'from' reserved SQL keyword, and the parser
doesn't handle that case. For the record, here's the PG query (only
been tested in psql, not in the CMS due to the PDL parser issue):

query getBlock {
   QueueEntry queueEntry;
   do {
           id, item_id, parent_id, task, destination,
           time_queued, time_last_failed, fail_count,
           sort_order, in_process
           host_id = :hostId
       and ( fail_count = 0
             or extract(epoch from time_last_failed) < (extract(epoch
from now()) - :queueEntryRetryDelay) )
       and ( :maximumFailCount < 0
              or fail_count < :maximumFailCount )
       } map {
               queueEntry.id = id;
               queueEntry.itemId = item_id;
               queueEntry.parentId = parent_id;
               queueEntry.task = task;
               queueEntry.destination = destination;
               queueEntry.timeQueued = time_queued;
               queueEntry.timeLastFailed = time_last_failed;
               queueEntry.failCount = fail_count;
               queueEntry.sortOrder = sort_order;
               queueEntry.inProcess = in_process;

Comment 2 Richard Li 2004-02-19 22:44:15 UTC
SQLParser.jj updated @40602 to address 'from' issue.

PG & Oracle versions checked in @40604. There was also some minor Java
porting work required for Rickshaw to avoid using deprecated API
(Web.getHosts() is now deprecated in Rickshaw).

Comment 3 Richard Li 2004-02-19 22:53:13 UTC
I should have mentioned that 40604 includes integrations of 40509 and
40564, the PG fixes detailed above, and the Java updates.

Comment 4 Carsten Clasohm 2004-02-20 09:24:46 UTC
In general, applying a function to an indexed column should be avoided
in where clauses. Unless the optimizer is able to rewrite the
condition to have nothing but the indexed column on one side of the
condition, the index cannot be used.

Instead of using EXTRACT, the condition could be written like this in

time_last_failed < now() - (:queueEntryRetryDelay)::interval

For the above query, it makes no difference, because we have no index
on time_last_failed.

Comment 5 Richard Li 2004-02-20 14:53:24 UTC
Hm. I can't get that syntax to work:

postgres=# select time_last_failed < now() - (10)::interval from
ERROR:  Cannot cast type integer to interval
postgres=# select time_last_failed < now() - '10 seconds'::interval
from publis\
(0 rows)

It appears you have to treat the queueEntryRetryDelay as a literal?

Comment 6 Carsten Clasohm 2004-02-20 15:11:26 UTC
'10' would also work, as "seconds" is the default. I only tried it in
pgsql, so I did not notice that 10 and not '10' would be passed to the

Comment 7 Richard Li 2004-02-20 16:12:39 UTC
So QueueManager does:

     query.setParameter("queueEntryRetryDelay", s_retryDelay);

where s_retryDelay is an Integer. This doesn't work for PG, because
the interval cast requires a JDBC type of a string. So, two options
that I can see:

1. Stick with the functions in the where clause.
2. Do something like:

if (DbHelper.getDB() = DB_POSTGRES ) {
 query.setParameter("queueEntryRetryDelay", s_retryDelay.toString());
} else {
 query.setParameter("queueEntryRetryDelay", s_retryDelay);

Carsten: any opinion as to whether or not the presumable performance
gain of #2 is worth it?

Comment 8 Richard Li 2004-02-20 16:13:23 UTC
(publish_to_fs_queue seems like it's a pretty small table (?), so
maybe functions are OK?)

Comment 9 Carsten Clasohm 2004-02-20 16:33:55 UTC
We can stick with the functions, since we don't have an index on the
column, and the table is indeed small. My comment was more about
coding style than about this particular query.

Comment 10 Richard Li 2004-02-20 16:37:38 UTC
danpb figured out a way to do the above (cast first to text, then to
interval). checked in @40627.

re: functions, i agree. i tried to avoid them, but i wasn't as clever
as you and dan in figuring out a way to not use them :).

Note You need to log in before you can comment on or make changes to this bug.