You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by hw...@apache.org on 2011/04/05 23:53:48 UTC
svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc: adm_ops.c
wc-queries.sql wc_db.c wc_db.h
Author: hwright
Date: Tue Apr 5 21:53:47 2011
New Revision: 1089257
URL: http://svn.apache.org/viewvc?rev=1089257&view=rev
Log:
Store changelist change notifications in the database while processing them.
Add another function which do notification of changelist changes.
(This is largely modeled on the same technique used for revert.)
* subversion/libsvn_wc/adm_ops.c
(svn_wc_set_changelist2): Remove the notification code in favor of the
notification function.
* subversion/libsvn_wc/wc-queries.sql
(STMT_CREATE_CHANGELIST_LIST, STMT_INSERT_CHANGELIST_LIST,
STMT_DELETE_CHANGELIST_LIST_RECURSIVE,
STMT_SELECT_CHANGELIST_LIST_RECURSIVE): New.
* subversion/libsvn_wc/wc_db.c
(set_changelist_txn): Populate the CHANGELIST_LIST table.
(svn_wc__db_op_set_changelist): Ensure the CHANGELIST_LIST table exists.
(svn_wc__db_changelist_list_notify): New.
* subversion/libsvn_wc/wc_db.h
(svn_wc__db_changelist_list_notify): New.
Modified:
subversion/trunk/subversion/libsvn_wc/adm_ops.c
subversion/trunk/subversion/libsvn_wc/wc-queries.sql
subversion/trunk/subversion/libsvn_wc/wc_db.c
subversion/trunk/subversion/libsvn_wc/wc_db.h
Modified: subversion/trunk/subversion/libsvn_wc/adm_ops.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/adm_ops.c?rev=1089257&r1=1089256&r2=1089257&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/adm_ops.c (original)
+++ subversion/trunk/subversion/libsvn_wc/adm_ops.c Tue Apr 5 21:53:47 2011
@@ -2158,7 +2158,6 @@ svn_wc_set_changelist2(svn_wc_context_t
void *notify_baton,
apr_pool_t *scratch_pool)
{
- svn_wc_notify_t *notify;
const char *existing_changelist;
svn_wc__db_kind_t kind;
@@ -2202,25 +2201,9 @@ svn_wc_set_changelist2(svn_wc_context_t
/* And tell someone what we've done. */
if (notify_func)
- {
- if (existing_changelist)
- {
- notify = svn_wc_create_notify(local_abspath,
- svn_wc_notify_changelist_clear,
- scratch_pool);
- notify->changelist_name = existing_changelist;
- notify_func(notify_baton, notify, scratch_pool);
- }
-
- if (changelist)
- {
- notify = svn_wc_create_notify(local_abspath,
- svn_wc_notify_changelist_set,
- scratch_pool);
- notify->changelist_name = changelist;
- notify_func(notify_baton, notify, scratch_pool);
- }
- }
+ SVN_ERR(svn_wc__db_changelist_list_notify(notify_func, notify_baton,
+ wc_ctx->db, local_abspath,
+ scratch_pool));
return SVN_NO_ERROR;
}
Modified: subversion/trunk/subversion/libsvn_wc/wc-queries.sql
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc-queries.sql?rev=1089257&r1=1089256&r2=1089257&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc-queries.sql (original)
+++ subversion/trunk/subversion/libsvn_wc/wc-queries.sql Tue Apr 5 21:53:47 2011
@@ -340,6 +340,30 @@ REPLACE INTO actual_node (
wc_id, local_relpath, parent_relpath, changelist)
VALUES (?1, ?2, ?3, ?4)
+-- STMT_CREATE_CHANGELIST_LIST
+DROP TABLE IF EXISTS changelist_list;
+CREATE TEMPORARY TABLE changelist_list (
+ wc_id INTEGER NOT NULL,
+ local_relpath TEXT NOT NULL,
+ notify INTEGER,
+ changelist TEXT NOT NULL
+ );
+CREATE INDEX changelist_list_index ON changelist_list(local_relpath);
+
+-- STMT_INSERT_CHANGELIST_LIST
+INSERT INTO changelist_list(wc_id, local_relpath, notify, changelist)
+VALUES (?1, ?2, ?3, ?4);
+
+-- STMT_DELETE_CHANGELIST_LIST_RECURSIVE
+DELETE FROM changelist_list
+WHERE local_relpath = ?1 OR local_relpath LIKE ?2 ESCAPE '#'
+
+-- STMT_SELECT_CHANGELIST_LIST_RECURSIVE
+SELECT local_relpath, notify, changelist
+FROM changelist_list
+WHERE local_relpath = ?1 or local_relpath LIKE ?2 ESCAPE '#'
+ORDER BY local_relpath
+
-- STMT_DELETE_ACTUAL_EMPTY
DELETE FROM actual_node
WHERE wc_id = ?1 AND local_relpath = ?2
Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=1089257&r1=1089256&r2=1089257&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
+++ subversion/trunk/subversion/libsvn_wc/wc_db.c Tue Apr 5 21:53:47 2011
@@ -3395,10 +3395,35 @@ set_changelist_txn(void *baton,
STMT_UPDATE_ACTUAL_CHANGELIST));
}
+ /* Run the update or insert query */
SVN_ERR(svn_sqlite__bindf(stmt, "iss", wcroot->wc_id, local_relpath,
new_changelist));
+ SVN_ERR(svn_sqlite__step_done(stmt));
- return svn_error_return(svn_sqlite__step_done(stmt));
+ /* Now, add a row to the CHANGELIST_LIST table, so we can later notify. */
+ /* ### TODO: This could just as well be done with a trigger, but for right
+ ### now, this is quick and dirty. */
+ if (existing_changelist)
+ {
+ SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
+ STMT_INSERT_CHANGELIST_LIST));
+ SVN_ERR(svn_sqlite__bindf(stmt, "isis", wcroot->wc_id, local_relpath,
+ svn_wc_notify_changelist_clear,
+ existing_changelist));
+ SVN_ERR(svn_sqlite__step_done(stmt));
+ }
+
+ if (new_changelist)
+ {
+ SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
+ STMT_INSERT_CHANGELIST_LIST));
+ SVN_ERR(svn_sqlite__bindf(stmt, "isis", wcroot->wc_id, local_relpath,
+ svn_wc_notify_changelist_set,
+ new_changelist));
+ SVN_ERR(svn_sqlite__step_done(stmt));
+ }
+
+ return SVN_NO_ERROR;
}
@@ -3431,6 +3456,9 @@ svn_wc__db_op_set_changelist(svn_wc__db_
NOT_IMPLEMENTED();
}
+ SVN_ERR(svn_sqlite__exec_statements(wcroot->sdb,
+ STMT_CREATE_CHANGELIST_LIST));
+
SVN_ERR(svn_wc__db_with_txn(wcroot, local_relpath, set_changelist_txn,
(void *) changelist, scratch_pool));
@@ -3441,6 +3469,64 @@ svn_wc__db_op_set_changelist(svn_wc__db_
svn_error_t *
+svn_wc__db_changelist_list_notify(svn_wc_notify_func2_t notify_func,
+ void *notify_baton,
+ svn_wc__db_t *db,
+ const char *local_abspath,
+ apr_pool_t *scratch_pool)
+{
+ svn_wc__db_wcroot_t *wcroot;
+ const char *local_relpath;
+ const char *like_arg;
+ svn_sqlite__stmt_t *stmt;
+ svn_boolean_t have_row;
+ apr_pool_t *iterpool = svn_pool_create(scratch_pool);
+
+ SVN_ERR(svn_wc__db_wcroot_parse_local_abspath(&wcroot, &local_relpath,
+ db, local_abspath, scratch_pool, iterpool));
+ VERIFY_USABLE_WCROOT(wcroot);
+
+ like_arg = construct_like_arg(local_relpath, scratch_pool);
+
+ SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
+ STMT_SELECT_CHANGELIST_LIST_RECURSIVE));
+ SVN_ERR(svn_sqlite__bindf(stmt, "ss", local_relpath, like_arg));
+ SVN_ERR(svn_sqlite__step(&have_row, stmt));
+ if (!have_row)
+ return svn_error_return(svn_sqlite__reset(stmt)); /* optimise for no row */
+
+ while (have_row)
+ {
+ const char *notify_relpath = svn_sqlite__column_text(stmt, 0, NULL);
+ svn_wc_notify_action_t action = svn_sqlite__column_int64(stmt, 1);
+ svn_wc_notify_t *notify;
+ const char *notify_abspath;
+
+ svn_pool_clear(iterpool);
+
+ notify_abspath = svn_dirent_join(wcroot->abspath, notify_relpath,
+ iterpool);
+ notify = svn_wc_create_notify(notify_abspath, action, iterpool);
+ notify->changelist_name = svn_sqlite__column_text(stmt, 2, NULL);
+ notify_func(notify_baton, notify, iterpool);
+
+ SVN_ERR(svn_sqlite__step(&have_row, stmt));
+ }
+ SVN_ERR(svn_sqlite__reset(stmt));
+
+ SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
+ STMT_DELETE_CHANGELIST_LIST_RECURSIVE));
+ SVN_ERR(svn_sqlite__bindf(stmt, "ss", local_relpath, like_arg));
+ SVN_ERR(svn_sqlite__step_done(stmt));
+
+ svn_pool_destroy(iterpool);
+
+ return SVN_NO_ERROR;
+
+}
+
+
+svn_error_t *
svn_wc__db_op_mark_conflict(svn_wc__db_t *db,
const char *local_abspath,
apr_pool_t *scratch_pool)
Modified: subversion/trunk/subversion/libsvn_wc/wc_db.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.h?rev=1089257&r1=1089256&r2=1089257&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc_db.h (original)
+++ subversion/trunk/subversion/libsvn_wc/wc_db.h Tue Apr 5 21:53:47 2011
@@ -1207,6 +1207,18 @@ svn_wc__db_op_set_changelist(svn_wc__db_
svn_depth_t depth,
apr_pool_t *scratch_pool);
+/* Make changelist mod notifications for all paths in the changelist list
+ * that are equal to or below LOCAL_ABSPATH.
+ *
+ * Removes the cooresponding rows from the changelist list.
+ */
+svn_error_t *
+svn_wc__db_changelist_list_notify(svn_wc_notify_func2_t notify_func,
+ void *notify_baton,
+ svn_wc__db_t *db,
+ const char *local_abspath,
+ apr_pool_t *scratch_pool);
+
/* ### caller maintains ACTUAL. we're just recording state. */
/* ### we probably need to record details of the conflict. how? */
Re: svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c wc-queries.sql wc_db.c wc_db.h
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Tue, Apr 5, 2011 at 6:09 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: hwright@apache.org [mailto:hwright@apache.org]
>> Sent: dinsdag 5 april 2011 23:54
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc:
>> adm_ops.c wc-queries.sql wc_db.c wc_db.h
>>
>> Author: hwright
>> Date: Tue Apr 5 21:53:47 2011
>> New Revision: 1089257
>>
>> URL: http://svn.apache.org/viewvc?rev=1089257&view=rev
>> Log:
>> Store changelist change notifications in the database while processing them.
>> Add another function which do notification of changelist changes.
>>
>> (This is largely modeled on the same technique used for revert.)
>
>
>> + /* ### TODO: This could just as well be done with a trigger, but for right
>> + ### now, this is quick and dirty. */
>> + if (existing_changelist)
>> + {
>> + SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
>> + STMT_INSERT_CHANGELIST_LIST));
>> + SVN_ERR(svn_sqlite__bindf(stmt, "isis", wcroot->wc_id, local_relpath,
>> + svn_wc_notify_changelist_clear,
>> + existing_changelist));
>
> 'i' in our bind function assumes 64 bit integers, while the enum is just 32 bit.
>
>> + SVN_ERR(svn_sqlite__step_done(stmt));
>> + }
>> +
>> + if (new_changelist)
>> + {
>> + SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
>> + STMT_INSERT_CHANGELIST_LIST));
>> + SVN_ERR(svn_sqlite__bindf(stmt, "isis", wcroot->wc_id, local_relpath,
>> + svn_wc_notify_changelist_set,
>> + new_changelist));
>> + SVN_ERR(svn_sqlite__step_done(stmt));
>> + }
>
> Same here.
>
> This breaks the build on systems that don't use 64 bit for int. (So this breaks 32 bit unix and 32 and 64 bit Windows)
I see you fixed these in r1089279. Thanks!
-Hyrum
Re: svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc: adm_ops.c wc-queries.sql wc_db.c wc_db.h
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> This breaks the build on systems that don't use 64 bit for int. (So
> this breaks 32 bit unix and 32 and 64 bit Windows)
Very few platforms use 64-bit int, 64-bit Linux does not. It's still a
bug, but not always (ever?) visible on 64-bit Linux due to the calling
convention of the ABI.
--
Philip
Re: svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c wc-queries.sql wc_db.c wc_db.h
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Tue, Apr 5, 2011 at 6:09 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: hwright@apache.org [mailto:hwright@apache.org]
>> Sent: dinsdag 5 april 2011 23:54
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc:
>> adm_ops.c wc-queries.sql wc_db.c wc_db.h
>>
>> Author: hwright
>> Date: Tue Apr 5 21:53:47 2011
>> New Revision: 1089257
>>
>> URL: http://svn.apache.org/viewvc?rev=1089257&view=rev
>> Log:
>> Store changelist change notifications in the database while processing them.
>> Add another function which do notification of changelist changes.
>>
>> (This is largely modeled on the same technique used for revert.)
>
>
>> + /* ### TODO: This could just as well be done with a trigger, but for right
>> + ### now, this is quick and dirty. */
>> + if (existing_changelist)
>> + {
>> + SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
>> + STMT_INSERT_CHANGELIST_LIST));
>> + SVN_ERR(svn_sqlite__bindf(stmt, "isis", wcroot->wc_id, local_relpath,
>> + svn_wc_notify_changelist_clear,
>> + existing_changelist));
>
> 'i' in our bind function assumes 64 bit integers, while the enum is just 32 bit.
>
>> + SVN_ERR(svn_sqlite__step_done(stmt));
>> + }
>> +
>> + if (new_changelist)
>> + {
>> + SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
>> + STMT_INSERT_CHANGELIST_LIST));
>> + SVN_ERR(svn_sqlite__bindf(stmt, "isis", wcroot->wc_id, local_relpath,
>> + svn_wc_notify_changelist_set,
>> + new_changelist));
>> + SVN_ERR(svn_sqlite__step_done(stmt));
>> + }
>
> Same here.
>
> This breaks the build on systems that don't use 64 bit for int. (So this breaks 32 bit unix and 32 and 64 bit Windows)
I see you fixed these in r1089279. Thanks!
-Hyrum
RE: svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc: adm_ops.c wc-queries.sql wc_db.c wc_db.h
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: hwright@apache.org [mailto:hwright@apache.org]
> Sent: dinsdag 5 april 2011 23:54
> To: commits@subversion.apache.org
> Subject: svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc:
> adm_ops.c wc-queries.sql wc_db.c wc_db.h
>
> Author: hwright
> Date: Tue Apr 5 21:53:47 2011
> New Revision: 1089257
>
> URL: http://svn.apache.org/viewvc?rev=1089257&view=rev
> Log:
> Store changelist change notifications in the database while processing them.
> Add another function which do notification of changelist changes.
>
> (This is largely modeled on the same technique used for revert.)
> + /* ### TODO: This could just as well be done with a trigger, but for right
> + ### now, this is quick and dirty. */
> + if (existing_changelist)
> + {
> + SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
> + STMT_INSERT_CHANGELIST_LIST));
> + SVN_ERR(svn_sqlite__bindf(stmt, "isis", wcroot->wc_id, local_relpath,
> + svn_wc_notify_changelist_clear,
> + existing_changelist));
'i' in our bind function assumes 64 bit integers, while the enum is just 32 bit.
> + SVN_ERR(svn_sqlite__step_done(stmt));
> + }
> +
> + if (new_changelist)
> + {
> + SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
> + STMT_INSERT_CHANGELIST_LIST));
> + SVN_ERR(svn_sqlite__bindf(stmt, "isis", wcroot->wc_id, local_relpath,
> + svn_wc_notify_changelist_set,
> + new_changelist));
> + SVN_ERR(svn_sqlite__step_done(stmt));
> + }
Same here.
This breaks the build on systems that don't use 64 bit for int. (So this breaks 32 bit unix and 32 and 64 bit Windows)
Bert
Re: svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc: adm_ops.c wc-queries.sql wc_db.c wc_db.h
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> Note: beyond the review below, I'm registering a general concern
> around this notion of "hey! let's drop off this data into some global
> variables^W^Woh, I mean the wc state database... and then some time
> later, we'll perform some operation around them".
>
> This is *so* ripe for danger, I'm depressed.
>
> Honestly. This "temporary table" is just another name for "global
> variable". "Results are collected in a table". Really? REALLY? How is
> that different from a global? Functions going this way and that,
> dropping off rows in the table as a *side effect*, and then along some
> other code path later in the control flow, we decide to pick them back
> up and run with them.
>
> Stinky.
I started doing this for revert. In the database a recursive revert is
something like:
DELETE FROM nodes WHERE local_relpath LIKE ... AND op_depth > ...
DELETE FROM actual_node WHERE local_relpath LIKE ...
It doesn't make sense to do that node-by-node, for a copied tree it's
wrong to revert a parent before a child or a child before a parent.
It's also painfully slow to do it node-by-node. Try a recursive propset
on a tree followed by a revert, that's a database only operation and the
database change happens in seconds as a single transaction, but it
crawls on a network disk when done node-by-node.
Given that the recursive operation is a much better way to do it, how
does notification work? The caller can't simply do it after the delete,
as once the rows have been deleted it's too late to determine which rows
were affected. The delete function has to somehow tell the caller which
rows were affected. It could allocate memory and build a list of rows
before deleting them, essentially that would be duplicating a chunk of
the database into memory (revert needs more than just the local_relpath,
it needs the conflict file names as well). During the subsequent walk
over the working files it is necessary to query the "list" for a given
path, so the "list" needs to be some sort of tree or table.
Doing all that memory management and indexing by hand is silly when we
have a database to do it for us. The temporary tables are not exactly
global, they are per-connection to the database. The function that
populates the table and the function that queries the table are both
called from within the same public svn_wc_xxx interface. So the table
doesn't have any significance outside that function.
What alternative implementation do you suggest?
--
Philip
Re: svn commit: r1089257 - in
/subversion/trunk/subversion/libsvn_wc: adm_ops.c wc-queries.sql wc_db.c
wc_db.h
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Hyrum K Wright wrote on Wed, Apr 06, 2011 at 11:44:47 -0500:
> On Wed, Apr 6, 2011 at 7:58 AM, Greg Stein <gs...@gmail.com> wrote:
> >...
> >>...
> >> +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Tue Apr 5 21:53:47 2011
> >>...
> >> svn_error_t *
> >> +svn_wc__db_changelist_list_notify(svn_wc_notify_func2_t notify_func,
> >> + void *notify_baton,
> >> + svn_wc__db_t *db,
> >> + const char *local_abspath,
> >> + apr_pool_t *scratch_pool)
> >> +{
> >
> > Should the contents of this function be transacted?
>
> Not needed: the contents of the temporary table are only visible to
> the database connection that created them (and the table itself will
> automatically disappear when the connection is closed). As a result,
> no other connection or thread could interact with this table while
> this function is running.
>
Using a transaction is twice as fast for me in the following testcsae:
[[[
% cat foo.sh
#!/bin/zsh
x=`for i in $(seq 100000); do echo "insert into foo(i) values($i);"; done`
y="create temporary table foo (i integer); create index ii on foo(i);"
time sqlite3 :memory: <<< "$y; $x;"
time sqlite3 :memory: <<< "$y; BEGIN; $x; COMMIT;"
% zsh -e foo.sh
sqlite3 :memory: <<< "$y; $x;" 1.42s user 1.23s system 97% cpu 2.707 total
sqlite3 :memory: <<< "$y; BEGIN; $x; COMMIT;" 0.74s user 0.01s system 99% cpu 0.745 total
]]]
(the <<< operator is a one-line heredoc, 'redirect this string to the
process's stdin)
> -Hyrum
Re: svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c wc-queries.sql wc_db.c wc_db.h
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Wed, Apr 6, 2011 at 7:58 AM, Greg Stein <gs...@gmail.com> wrote:
>...
>>...
>> +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Tue Apr 5 21:53:47 2011
>>...
>> svn_error_t *
>> +svn_wc__db_changelist_list_notify(svn_wc_notify_func2_t notify_func,
>> + void *notify_baton,
>> + svn_wc__db_t *db,
>> + const char *local_abspath,
>> + apr_pool_t *scratch_pool)
>> +{
>
> Should the contents of this function be transacted?
Not needed: the contents of the temporary table are only visible to
the database connection that created them (and the table itself will
automatically disappear when the connection is closed). As a result,
no other connection or thread could interact with this table while
this function is running.
-Hyrum
Re: svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c wc-queries.sql wc_db.c wc_db.h
Posted by Branko Čibej <br...@e-reka.si>.
On 06.04.2011 16:17, C. Michael Pilato wrote:
> I'd love to see a better suggestion, ideally one that avoids the glut of new
> queries and bifurcation of the "depth" concept into 3 or 4 different handler
> functions such as what I recall was the case for what should have been a
> relatively simple 'proplist' operation.
The alternative is to recursively walk the database in code, keeping a
long-term lock and throwing performance to the dogs.
-- Brane
Re: svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c wc-queries.sql wc_db.c wc_db.h
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 04/06/2011 08:58 AM, Greg Stein wrote:
> Note: beyond the review below, I'm registering a general concern
> around this notion of "hey! let's drop off this data into some global
> variables^W^Woh, I mean the wc state database... and then some time
> later, we'll perform some operation around them".
>
> This is *so* ripe for danger, I'm depressed.
>
> Honestly. This "temporary table" is just another name for "global
> variable". "Results are collected in a table". Really? REALLY? How is
> that different from a global?
I don't think anyone would claim that this is different (as an approach) to
a global variable. IIUC, it's for the express goal of avoiding having to
keep potentially large amounts of in-memory state that this approach was taken.
I'd love to see a better suggestion, ideally one that avoids the glut of new
queries and bifurcation of the "depth" concept into 3 or 4 different handler
functions such as what I recall was the case for what should have been a
relatively simple 'proplist' operation. (Maybe that's been cleaned up
recently, or maybe I misunderstood the code in the first place -- I can't
say for sure.)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c wc-queries.sql wc_db.c wc_db.h
Posted by Greg Stein <gs...@gmail.com>.
Note: beyond the review below, I'm registering a general concern
around this notion of "hey! let's drop off this data into some global
variables^W^Woh, I mean the wc state database... and then some time
later, we'll perform some operation around them".
This is *so* ripe for danger, I'm depressed.
Honestly. This "temporary table" is just another name for "global
variable". "Results are collected in a table". Really? REALLY? How is
that different from a global? Functions going this way and that,
dropping off rows in the table as a *side effect*, and then along some
other code path later in the control flow, we decide to pick them back
up and run with them.
Stinky.
See my further review below on the implementation of this Badness:
On Tue, Apr 5, 2011 at 17:53, <hw...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_wc/wc-queries.sql Tue Apr 5 21:53:47 2011
> @@ -340,6 +340,30 @@ REPLACE INTO actual_node (
> wc_id, local_relpath, parent_relpath, changelist)
> VALUES (?1, ?2, ?3, ?4)
>
> +-- STMT_CREATE_CHANGELIST_LIST
> +DROP TABLE IF EXISTS changelist_list;
> +CREATE TEMPORARY TABLE changelist_list (
> + wc_id INTEGER NOT NULL,
> + local_relpath TEXT NOT NULL,
> + notify INTEGER,
> + changelist TEXT NOT NULL
> + );
> +CREATE INDEX changelist_list_index ON changelist_list(local_relpath);
Why doesn't this have wc_id?
> +
> +-- STMT_INSERT_CHANGELIST_LIST
> +INSERT INTO changelist_list(wc_id, local_relpath, notify, changelist)
> +VALUES (?1, ?2, ?3, ?4);
> +
> +-- STMT_DELETE_CHANGELIST_LIST_RECURSIVE
> +DELETE FROM changelist_list
> +WHERE local_relpath = ?1 OR local_relpath LIKE ?2 ESCAPE '#'
> +
> +-- STMT_SELECT_CHANGELIST_LIST_RECURSIVE
> +SELECT local_relpath, notify, changelist
> +FROM changelist_list
> +WHERE local_relpath = ?1 or local_relpath LIKE ?2 ESCAPE '#'
> +ORDER BY local_relpath
And these two statements should have wc_id.
>...
> +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Tue Apr 5 21:53:47 2011
>...
> svn_error_t *
> +svn_wc__db_changelist_list_notify(svn_wc_notify_func2_t notify_func,
> + void *notify_baton,
> + svn_wc__db_t *db,
> + const char *local_abspath,
> + apr_pool_t *scratch_pool)
> +{
Should the contents of this function be transacted?
>...
Cheers,
-g
RE: svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc: adm_ops.c wc-queries.sql wc_db.c wc_db.h
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: hwright@apache.org [mailto:hwright@apache.org]
> Sent: dinsdag 5 april 2011 23:54
> To: commits@subversion.apache.org
> Subject: svn commit: r1089257 - in /subversion/trunk/subversion/libsvn_wc:
> adm_ops.c wc-queries.sql wc_db.c wc_db.h
>
> Author: hwright
> Date: Tue Apr 5 21:53:47 2011
> New Revision: 1089257
>
> URL: http://svn.apache.org/viewvc?rev=1089257&view=rev
> Log:
> Store changelist change notifications in the database while processing them.
> Add another function which do notification of changelist changes.
>
> (This is largely modeled on the same technique used for revert.)
> + /* ### TODO: This could just as well be done with a trigger, but for right
> + ### now, this is quick and dirty. */
> + if (existing_changelist)
> + {
> + SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
> + STMT_INSERT_CHANGELIST_LIST));
> + SVN_ERR(svn_sqlite__bindf(stmt, "isis", wcroot->wc_id, local_relpath,
> + svn_wc_notify_changelist_clear,
> + existing_changelist));
'i' in our bind function assumes 64 bit integers, while the enum is just 32 bit.
> + SVN_ERR(svn_sqlite__step_done(stmt));
> + }
> +
> + if (new_changelist)
> + {
> + SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
> + STMT_INSERT_CHANGELIST_LIST));
> + SVN_ERR(svn_sqlite__bindf(stmt, "isis", wcroot->wc_id, local_relpath,
> + svn_wc_notify_changelist_set,
> + new_changelist));
> + SVN_ERR(svn_sqlite__step_done(stmt));
> + }
Same here.
This breaks the build on systems that don't use 64 bit for int. (So this breaks 32 bit unix and 32 and 64 bit Windows)
Bert