Nicolas Williams wrote: > On Wed, Mar 05, 2008 at 11:53:58AM +0000, Julian Pullen wrote: >> >> Nicolas Williams wrote: >>> On Tue, Mar 04, 2008 at 10:35:15AM -0600, Nicolas Williams wrote: >>>> On Tue, Mar 04, 2008 at 04:26:50PM +0000, Julian Pullen wrote: >>>>> http://cr.opensolaris.org/~pullenj/observability/ >>>> Only two files to go (dbutils.c and idmap.c). >>> - dbutils.c:init_dbs() >>> >>> There's no need to NULL-terminate the sql[] array -- init_db_instance() >>> should already know that the max index is that given by its schema >>> version argument. >> I think it is good practice to leave it in as I fell over this not >> realizing that I needed to expand the SQL array. > > Arguably your tripping over it was a *feature* -- it might have taken > more effort to figure out the need to bump the schema version number > otherwise. > > Redundancy is bad. Either get rid of the schema version number or the > NULL terminator for that array. > >>> - dbutils.c:lookup_cache_sid2pid() >>> >>> Methinks there's got to be a better way to write the SQL code here. >>> >>> Two options: a) mprintf the additional column list in when >>> IDMAP_REQ_FLG_MAPPING_INFO is given, or b) always request the "how" >>> columns and ignore them when IDMAP_REQ_FLG_MAPPING_INFO is not given. >>> >>> I think I prefer (b). I'm thinking that whatever negative >>> performance impact here will be very small and anyways, looking ahead >>> to when we move to SQLite3 we will gain back a lot more perf by using >>> pre-compiled SQL statements. >> Lets leave this for now and see what others think. I don't want to effect >> performance. > > I don't like having to make sure that two very large SQL statements and > surrounding code are correct when just one will do. Please pick one of > (a) or (b), and don't worry about performance. Premature optimization > and all that. (Keep in mind that SQLite2 keeps all the fields of a > table row together, so fetching more or fewer of them has no impact on > I/O, only an impact on memory allocations.) > > Nico
Sold. OK I will make all the SQL lookups retieve the mapping info.
