Permalink
Commits on Jan 11, 2018
  1. ✂️ remove intermediate variable

    tenderlove committed Jan 11, 2018
  2. Pass name in to aggregator factory method

    tenderlove committed Jan 11, 2018
    We can determine the name of the aggregate function in Ruby, so let's do
    that.
  3. Fix SEGV

    tenderlove committed Jan 11, 2018
    `i` was not initialized, so just use 0 for argc == 1
  4. Merge branch 'fix-aggregate-functions' of https://github.com/jmetter/…

    tenderlove committed Jan 11, 2018
    …sqlite3-ruby into jmetter-fix-aggregate-functions
    
    * 'fix-aggregate-functions' of https://github.com/jmetter/sqlite3-ruby:
      fix aggregators in Ruby 1.9
      Aggregators backed by ruby arrays, no xDestroy callback
      define_aggregator respects step's arity again
      avoid leak in define_aggregator2
      avoid mixed declarations
      No PRIsVALUE in ruby 1.9.3
      parameters, not grouped expr
      protect funcalls in aggregate callbacks
      test aggregate handlers raise in new/finalize
      avoid heap allocation for single VALUE
      test aggregate with invalid arity
      use sqlite3_aggregate_context()
      remove misplaced asterisk from xcalloc call
      test for aggregate functions used with group by
      fix invalid memory access
      simple linked list implementation
      tests to demonstrate issues with aggregate function handling
      Move aggregate tests to a new file
  5. Revert "Merge pull request #215 from gazayas/master"

    tenderlove committed Jan 11, 2018
    This reverts commit 0617a13, reversing
    changes made to 1d146d2.
  6. Merge pull request #213 from junaruga/feature/travis-ruby24

    tenderlove committed Jan 11, 2018
    Add Ruby 2.4.1 to Travis CI
  7. Merge pull request #214 from larskanis/add-ruby24-and-mingw-tag

    tenderlove committed Jan 11, 2018
    Add ruby24 to fat binaries and add mingw dependency tag
  8. Merge pull request #215 from gazayas/master

    tenderlove committed Jan 11, 2018
    Fix typo in ext/database.c
  9. Merge pull request #219 from gazayas/add_test

    tenderlove committed Jan 11, 2018
    test_statement.rb: add bind blob test
  10. Merge pull request #223 from bjfish/convert-encoding-to-num

    tenderlove committed Jan 11, 2018
    Convert encoding int to VALUE argument
Commits on Sep 23, 2017
  1. Convert encoding int to VALUE argument

    bjfish committed Sep 23, 2017
Commits on May 26, 2017
  1. test_statement.rb: add bind blob test

    gazayas committed May 26, 2017
Commits on Apr 17, 2017
  1. Fix typo in ext/database.c

    gazayas committed Apr 17, 2017
Commits on Apr 4, 2017
  1. Add msys2 library dependency tag in gem metadata.

    larskanis committed Apr 4, 2017
    RubyInstaller2 supports metadata tags for installation of dependent
    MSYS2/MINGW libraries. The sqlite3 source gem requires the sqlite3
    library to be installed on the system, which the gem installer takes
    care about, when this tag is set.
    
    The feature is documented here:
    https://github.com/oneclick/rubyinstaller2/wiki/For-gem-developers#msys2-library-dependency
  2. Update rake-compiler and rake-compiler-dock to support ruby-2.4 fat b…

    larskanis committed Apr 4, 2017
    …inary gems.
    
    This drops support for ruby-2.0 binary gems.
Commits on Mar 29, 2017
  1. Add Ruby 2.4.1 to Travis CI.

    junaruga committed Mar 29, 2017
  2. fix aggregators in Ruby 1.9

    jmetter committed Mar 29, 2017
    rb_class_new generatos class with undefined allocator in ruby 1.9. Calling Class.new is also a one-liner and works in ruby 1.9 and 2.x.
  3. Aggregators backed by ruby arrays, no xDestroy callback

    jmetter committed Mar 28, 2017
    All data is now managed in plain ruby objects referenced from ruby
    arrays. Instance variables are prefixed with "-" to make them
    inaccessible from scripts.
    
    I ran into the finalization issue in `deallocate()`: When
    `deallocate()` calls `sqlite3_close()` the object passed to
    `sqlite3_create_function` must still be alive. Otherwise the
    `xDestroy()` handler will attempt to destroy something that is not
    around any more. Unfortunately, when `deallocate()` is invoked, the GC
    might already have released the aggregator wrappers, because the
    SQLite3::Database ruby object is already dead at that point. The only
    way to circumvent this, that I came up with, would have been to
    reference the aggregator wrappers from a global root, e.g. by
    referencing a ruby array with `rb_gc_register_address()`. Given that
    the aim was to cut code size, I considered it more appropriate to do
    without the xDestroy callback all together. That is, I call the old
    sqlite3_create_function API again.
  4. define_aggregator respects step's arity again

    jmetter committed Mar 1, 2017
    This has been undocumented behavior before: while create_aggregate and
    create_aggregate_handler effectively used an arity of -1, the library
    user could actually define functions with a different arity by calling
    define_aggregator directly. I was not aware of this behavior and
    therefore broke it by forcing the arity to -1. Now I added the required
    shim and a test.
  5. avoid leak in define_aggregator2

    jmetter committed Mar 1, 2017
    Call StringValueCStr before xcalloc but last of all calls into ruby.
    This way we do not leak memory should name.to_s raise or return a
    String with interior null.
  6. avoid mixed declarations

    jmetter committed Feb 27, 2017
    Silence some warnings.
  7. No PRIsVALUE in ruby 1.9.3

    jmetter committed Feb 27, 2017
  8. parameters, not grouped expr

    jmetter committed Feb 27, 2017
  9. protect funcalls in aggregate callbacks

    jmetter committed Feb 27, 2017
    We call +new+, +step+ and +finalize+ in callback context from
    sqlite3_step.
    If one of these function raises, we have to deal with the following
    issues:
    - all three functions (may) leak memory from xcalloc in this case
    - the checks I inserted in 325ac74
    result in
      a rb_fatal() if +new+ raises.
    - SQLite surely does not expect its callbacks to far-jump out of
    sqlite3_step.
      I don't if that causes any inconsistencies in SQLite's internal data
    structures.
    
    We address these issues by protecting these three funcalls with
    rb_protect, which suppresses the exceptions. Statement.step is the
    point where suppressed exceptions should be re-raised. As I could not
    find an easy way to transport the status value from rb_protect to
    Statement.step (sqlite3_set_error_code would be possible but fragile),
    I instead re-raise the exception from rb_errcode(). In principal, this
    may raise any exception in $!, but I don't see how this would affect
    normal client code.
    
    I also set sqlite3_result_error in finalize to indicate an exception,
    although the user should see the raised exception instead. It would
    certainly be more consequent to call sqlite3_result_error as soon as we
    catch an exception. I guess this would stop SQLite from doing any more
    useless work and abort the query. But, as I already wrote, I am unsure
    about the consequences for memory lifetime in that case, so it seems
    safer to me to defer any error until finalize.
  10. avoid heap allocation for single VALUE

    jmetter committed Feb 27, 2017
    A tiny optimization for the common case that an aggregate function
    takes only one value.
  11. test aggregate with invalid arity

    jmetter committed Feb 27, 2017
    Arities outside [-1,127] invoke undefined behavior.
  12. use sqlite3_aggregate_context()

    jmetter committed Feb 27, 2017
    This commit addresses the issue that using the same aggregate function
    twice in a select statement mixes up the execution contexts in the
    AggregateHandler. While resetting the aggregate Handler in the
    finalize() method emulates the context lifetime in the other cases
    (group by, independent select), resetting is a fragile approach.
    Instead sqlite3_aggregate_context() is the function in the SQLite API
    that should be used to separate contexts.
    
    sqlite3_create_context() is a bit weird though: it is memory allocator
    that returns the same memory when called from a context that already
    called sqlite3_create_context once. Equally strange is that there is no
    destroy() callback -- sqlite will clear up the memory when it does not
    need it any more and won't tell us. Because a second allocator is
    difficult to combine with Ruby's GC and because I do not have the
    lifetime control, I use it only to store a pointer to a heap allocated
    aggregator_instance_t structure. This structure holds the VALUE for the
    Ruby instance that belongs to the aggregate function's execution
    context. The structure forms a linked list rooted in the
    aggregate_wrapper_t for the function it as an instance of, so the
    instance can be GC marked.
    
    SQLite's API left me with no choice but to assume that every execution
    context ends with a single call to finalize(). At that point I unlink
    and destroy the aggregate_instance_t. I never observed SQLite doing
    anything else. There are checks in place to catch if SQLite does call
    finalize() a second time. If it does not ever call finalize(), we leak
    memory. If someone wants to use sqlite3_result_error, he or she should
    check if the invocation of sqlite3_result_error is also the right place
    to destroy the aggregate_instance_t.
    
    This commit comes with one rather big change: now sqlite3-ruby
    allocates the instances of the aggregate handler in the C code. This
    forced me to change the interface, because the existing interface
    passed only the instance object, not the factory, to the C code. To
    that end, I introduced a new private method Database.define_aggregator2
    that takes the factory as it sole parameter. Its like
    Database.create_aggregate_handler but without the FunctionProxy "ctx"
    object that serves no useful purpose. I wrote a new Ruby shim for the
    old Database.define_aggregator and adopted Database.create_aggregate
    and Database.create_aggregate_handler to the new interface between Ruby
    and C. Generally I was careful to keep the interface as it is.
    
    There is a tiny API change that I could not help avoid:
    As Database.define_aggregator only receives one object, but it needs
    one per execution context, the only option i saw was to use the
    provided one as template and give every new instance a clone. This
    means, however, that the template must provide a suitable clone. As
    every ordinary ruby object already provides one, this should not pose
    problems in practice, though.
    
    And of course, people who were getting wrong results from their
    aggregate functions suddenly get the right one, which may also come as
    a surprise to them.
  13. remove misplaced asterisk from xcalloc call

    jmetter committed Feb 27, 2017
    This is clearly an array of VALUE, not of VALUE*. As it should hold
    sizeof(VALUE) == sizeof(VALUE*) this does not really matter though.
  14. test for aggregate functions used with group by

    jmetter committed Feb 26, 2017
    I missed this in the initial set of unit tests. #44 handles this case
    already. Keep in mind though that is just the effect of the order in
    which SQLite calls the step() and finalize() functions and could
    theoretically change.
  15. fix invalid memory access

    jmetter committed Feb 26, 2017
    As I already wrote in an earlier commit message, the memory access
    violation occurs after the library user installs a second aggregate
    handler. In that case, Database.@aggregate is overwritten and
    therefore no references to the AggregateHandler remain. The simplest
    fix would be to change @aggregator (or @agregator) into a Ruby array of
    all AggregateHandlers ever installed. I decided against that because I
    think its cleaner when SQLite controls the lifetime of the handlers. To
    that end the sqlite developers extended the sqlite3_create_function API
    with the xDestroy callback. The extended "v2" Version is available
    since 3.7.3 (2010-10-08). I installed a fallback to the old version,
    should the new function be unavailable. While it is possible to remove
    elements from an @aggregator ruby array while in xDestroy callback,
    this was clumsy and difficult to do correctly under undefined
    finalization order. I came to the conclusion that its easier to manage
    the lifetime completely in C and only provide the appropriate
    rb_gc_mark function to SQLite3::Database. The remaining issue is to
    make the VALUEs for the AggregateHandlers (and later their instances)
    visible to the mark function. To that end, I introduced the
    aggregator_wrapper struct that holds the VALUE and the linked list
    pointers. I replaced @aggregator with the _sqlite3Ruby.aggregators
    linked list head.
    
    I used valgrind to check that memory accesses are sane now. Before this
    patch, I saw loads from previously free'd memory that disappeared.
    There are still lots of warnings about use initialized values, but this
    is unrelated and most likely an internal issue with MRI 2.0.0.
    
    As part of this patch, I moved the aggregate function related code into
    its own files "aggregator.[ch]".
  16. simple linked list implementation

    jmetter committed Feb 25, 2017
    To manage the active references to the aggregate handler and its
    instances, I need some list/array/vector. I have used Ruby's array for
    that, but wrapping the rb_sqlite3_aggregator_wrapper with a new, hidden
    Ruby class turned out more complex than I initially expected.
    Especially the undefined finalization order of the GC posed problems on
    shutdown. Consequently I decided that it would be better to handle the
    datastructures completely in C there I have full control over
    the destruction order.
  17. tests to demonstrate issues with aggregate function handling

    jmetter committed Feb 21, 2017
    Added tests to demonstrate three issues with the way sqlite3-ruby
    currently handles aggregate functions:
    
    (1) Defining a second aggregate function and using both results in
    memory violation. The problem is that `rb_iv_set(self, "@agregator",
    aggregator);`   (database.c:399) just overwrites the last reference to
    the first  AggregateHandler, but SQLite still holds a pointer to it and
    will follow   the pointer if the first aggregate function is used in a
    query.
      The most straight-forward fix is to insert the aggregator in a ruby
    array and never release it -- similar to how its done for functions.
    
    (2) Using the same aggregate function twice in a query mixes up values
    from both  columns. For example:
      `SELECT MYAGGREG(a), MYAGGREG(b) FROM foo;`
      Leads to: Myaggreg.step(a1); Myaggreg.step(b1); Myaggreg.step(a2);
      Myaggreg.step(b2); ... ; Myaggreg.finalize(), Myaggreg.finalize()
      The SQLite API expects the caller to differentiate between these
    chains  of invocation via `sqlite3_aggregate_context()`, but current
    sqlite3-ruby  does not account for that.
      #44 has been a work around for this in the special case that the first
      aggregation is finalized before the second started (separate queries).
      #161 does the analog for the function Proxy.
    
    (3) Documentation implies that library users shall explicitly set the
    arity of the function. Consequently the library user is likely to
    expect that this is passed down to SQLite, so he may define multiple
    functions with the same name but different arity and SQLite to use the
    most appropriate one  (documented sqlite feature). Unfortunately,
    sqlite3-ruby does not pass the arity to SQLite, which is surprising
    given that different arity-values  are accounted for in the C code. The
    problem is that sqlite3-ruby does not call the AggregateHandlers
    "arity" method but instead queries the arity of
      the "step" method from Ruby (sqlite3_obj_method_arity). Confusingly,
    this  is not the "step" provided by the library user, but the "step" of
    the  Proxy Wrapper classes introduced by Database.create_aggregate or
    Database.create_aggregate_handler. Both of these are just
    `step(*args)`,  so the arity reported to SQLite is always -1.
    
    Things not addressed:
      - #164 impossible to call FunctionProxy.set_error (and
    FunctionProxy.count).  because @driver does not exist (anymore)
      - Database.create_aggregate and the tests contain a 'text_rep' field,
    that  is never passed down to SQLite. It is not advertised in the
    documentation, though.
  18. Move aggregate tests to a new file

    jmetter committed Feb 21, 2017
    For the upcoming tests, I need a third column in the 'foo' table -- or
    alternatively a second table. Extending the existing table in
    `test_integration.rb` let some other tests fail. Given that
    `test_integration.rb` is already lengthy, I thought it would make
    things clearer if I separate the aggregate tests into their own file
    with their own setup/teardown methods.
Commits on Mar 23, 2017
  1. Merge pull request #142 from AutogrowSystems/markdown-faw

    tenderlove committed Mar 23, 2017
    Added a FAQ in Markdown