volkszaehler / volkszaehler.org

Open Source Smart Meter with focus on privacy - you remain the master of your data.
https://volkszaehler.org
GNU General Public License v3.0
212 stars 84 forks source link

Enhancement: add support for further Databases #60

Closed andig closed 10 years ago

andig commented 11 years ago

Initially, instead of a specific non-MySQL DB-implementation, I'd suggest researching the best way to introduce DB abstraction. Currently, Interpreter.php should be main focus.

andig commented 11 years ago

Hi Justin,

Du scheints ein Problem mit dem Issue zu haben.

so here we have the first issue that omits 90% of possibly useful

information:

  • the code USED to work on DBMSs other than mysql

-> Falsch. Wie ich bereits schrieb funktionierte der Code bzgl GroupBy bereits vorher nicht z.B. mit SQlite.

Siehe oben. Wenn Du keine Patches von mir willst sag es einfach. Anderenfalls verzichte bitte darauf einen "Schuldigen" zu suchen. So macht mir jedenfalls die Sache keinen Spass.

On Thu, Nov 14, 2013 at 2:49 PM, r00t- notifications@github.com wrote:

otherwise your intention is not clear. do you intend to:

  • restore the originally present support for non-mysql? or

Nochmal. Diesen Support gab es auch ursprünglich nicht. Ich suche Dir auch gerne noch die Codezeilen raus.

  • introduce a NEW abstraction layer to support yet more DBMSs? (i''d not consider that a useful project, given that we support(ed) enough of them as it is (was), and already have performance issues)

Exakt. Andere Projekte nutzen GitHub Issues vom Typ "enhancement" um solche Ideen zu dokumentieren. Damit kann jeder der neu reinkommt schauen was benötigt wird und sich so eines Themas annehmen. Noch einfacher wirds wenn der Maintainer die Issues dafür taggt (Bug, Enhancement, Wontfix etc)...

vg Andreas

r00t- commented 11 years ago

sorry, ich suche keinen schuldigen, mich stoerte nur, dass keine der infos aus dem thread hier auftauchten. ich hatte aus den kommentaren geschlossen, dass sqlite vorher mal funktionierte, sorry wenn ich mich da verlesen habe mir persoenlich ist auch egal, ob's mit was anderem als mysql funktioniert. und ich bin nicht justin

r3wald commented 11 years ago

Actually I don't know the problem Andreas tried to fix. As far as I understood it was a mixture of performance issues and "off by one" results. His fix at least excluded databases other than mysql from working.

I'm strongly against introducing some new abstraction layer. Doctrine ORM should be enough. From Doctrine ORMs documentation:

Object relational mapper (ORM) for PHP that sits on top of a powerful database abstraction layer (DBAL).

Justin originally used Postgresql (http://volkszaehler.org/volkszaehler.org.easterhegg.2010-04-02.pdf). The documentation talks of MySQL and PostgreSQL (http://wiki.volkszaehler.org/obsolete/setup?s[]=DB). There is at least interest in using SQLite. And with Doctrine we're just one revert/bugfix away from this. Doctrine only depends on PDO. So we could use any database with a PDO connector.

I suggest to make this project database independent, stick to Doctrine DQL, fix the actual problem and optimize for perfomance somewhere else.

https://www.google.de/search?q=premature+optimization

PS: In English, auf deutsch, wie Ihr wollt... Aber entscheidet Euch bitte :-)

andig commented 11 years ago

And with Doctrine we're just one revert/bugfix away from this. Doctrine only depends on PDO. So we could use any database with a PDO connector.

Stimmt einfach nicht. Wenigstens SQlite klappt nicht.

Ich würde das auch gerne fixen, suche aber einen guten Ansatz. DQL ist es aus meiner Sicht (bzw. wenn ich es mache..) nicht. Ich habe hier noch einen größeren Patch liegen den ich nur mit SQL realisieren kann.

Wie könnte man das stattdessen implementieren (oder: wie machen es andere)? Interpreter subclassen? SQL Code von einer weiteren Klasse providen lassen?

r3wald commented 11 years ago

So'n Scheiss. Ich hab eine schöne lange Antwort geschrieben und Github hat sie "verschluckt"... Wie auch immer, das da oben ist nicht korrekt. Ich hab seit mehreren Wochen SQLite als Datenbank in Benutzung und alles funktioniert. Justin hat Recht, es lief mal mit anderen Datenbanken. Quellen dafür hatte ich angeführt.

Ein "git diff" liefert bei mir folgendes:

diff --git a/lib/Interpreter/Interpreter.php b/lib/Interpreter/Interpreter.php
index 990bac8..c393170 100644
--- a/lib/Interpreter/Interpreter.php
+++ b/lib/Interpreter/Interpreter.php
@@ -148,7 +148,7 @@ abstract class Interpreter {
                        return new \EmptyIterator();

                // perform any optimizations, based on the actual number of rows
-               $stmt = $this->runSQL($sql, $sqlParameters);
+               $stmt = $this->conn->executeQuery($sql, $sqlParameters);

                return new DataIterator($stmt, $this->rowCount, $this->tupleCount);
        }

Interpreter kann man nicht weiter "subclassen". Das passiert nämlich schon. Das Problem hier ist, dass single responsibilty nicht beachtet wurde. Wenn die Klasse Daten transportiert, darf sie die nicht auch noch selbst aus der Datenbank holen. Das sollte eine andere Klasse machen. Und von der kann man dann gerne eine generische und eine MySQL-Variante bauen.

r00t- commented 11 years ago

nochmal: a) ich bin thorben, nicht justin b) ich finde/fand es durchaus gerechtfertigt, die datenbankabstraktion zu verwerfen, wenn sich damit die performance verbessern laesst.

ich denke das problem mit "single responsibility" ist, dass es duchaus sinn macht, einen teil der interpreter-arbeit direkt von der db machen zu lassen (zB das gruppieren), anstatt immer alle daten aus dem gewaehlten bereich aus der db zu laden und dann in php umherzuwuchten, das skaliert sicherlich schlechter. (das beides nicht optimal ist, ist ein anderes thema, vorschlaege das zu beheben sind willkommen.)

ich wuerde vorschlagen andi postet nochmal seine messergebnisse, und robert kann dann ja versuchen, eine andere erklaerung dafuer zu finden.

andig commented 11 years ago

Ich hab seit mehreren Wochen SQLite als Datenbank in Benutzung und alles funktioniert.

Dann mach bitte mal eine gruppierte Abfrage.

ich wuerde vorschlagen andi postet nochmal seine messergebnisse,

Um wieder auf den Boden der Tatsachen zurück zu finden habe ich in meinem Fork mal Travis für mysql, pgsql und sqlite aufgesetzt. Es scheint als würde mit SQlite nichtmal das Setup gelingen:

$ php misc/tools/doctrine orm:schema-tool:create --dump-sql > misc/sql/schema.sql
$ sh -c "if [ '$DB' = 'sqlite' ]; then cat misc/sql/schema.sql | sqlite3 sqlite.db3; fi;"
Error: incomplete SQL: CREATE UNIQUE INDEX ts_uniq ON data (channel_id, timestamp)

Erstmal keine weitere Analyse, das passiert mit Doctrine 2.2 wie derzeit im Master?

r3wald commented 11 years ago

@andig Folgendes führt sofort zum gewünschten Ergebnis

php misc/tools/doctrine orm:schema-tool:create

Dem erzeugten SQL-Dump fehlt einfach ein Semikolon am Ende. Ich vermute, das ist ein Bug in Doctrine. Bei MySQL ist dieses nämlich optional, nicht so bei SQLite.

Welche Abfrage genau möchtest Du denn sehen?

@r00t- Die Berechnung der Werte soll selbstverständlich weiter in der Datenbank erzeugen. Alles andere wäre Unsinn. Nur spricht nichts dagegen, ein Datenobjekt zu bauen, das einmalig über den Konstruktor bestückt wird und ansonsten nur Getter für die enthaltenen Daten hat:

Die Optimierungen von denen wir hier sprechen, haben mit so einem Datenobjekt nichts zu tun. Stattdessen kommen sie in die Klassen, die mit der DB reden und die Datenobjekte erzeugen.

PS: Sorry, wegen des Namens. Im ursprünglichen Kommentar wollte ich darauf verweisen, das Justin zu Anfang Postgresql verwendete. Das steht übrigens jetzt noch auf der Middleware-Seite im Wiki.

andig commented 11 years ago

Welche Abfrage genau möchtest Du denn sehen?

Versuch mal &groupby=hour. Bei https://travis-ci.org/andig/volkszaehler.org/jobs/14442765 sieht das gem. TestCase so aus:

1) DataCounterTest::testThreeDatapointsAggregationByHour
PDOException: SQLSTATE[42883]: Undefined function: 7 ERROR: function from_unixtime(bigint) does not exist
LINE 1: SELECT COUNT(DISTINCT YEAR(FROM_UNIXTIME(timestamp/1000)), D...
^
HINT: No function matches the given name and argument types. You might need to add explicit type casts.

Und dann den Funktionskopf anschauen:

/**
 * Builds sql query part for grouping data by date functions
 *
 * @param string $groupBy
 * @return string the sql part
 * @todo make compatible with: MSSql (Transact-SQL), Sybase, Firebird/Interbase, IBM, Informix, MySQL, Oracle, DB2, PostgreSQL, SQLite
 */
protected static function buildGroupBySQL($groupBy) {
andig commented 10 years ago

ich wuerde vorschlagen andi postet nochmal seine messergebnisse,

Habe ich jetzt nachgeholt. Testfall: 1 Channel, 1 Datensatz je Minute, 1 Jahr Daten = 500k Records.

Abfrage von nur 1 Monat Daten, Tuple gepackt: http://localhost/vz/middleware.php/data.json?uuid=00000000-0000-0000-0000-000000000000&from=1.1.2000&to=1.2.2000&tuples=1

Laufzeit mit originalem Code 1300-1600ms. Laufzeit mit angepasstem Code 600-800ms -> Speedup x2.

Da vzmon relativ viele Abfragen auf den Gesamtdatenbestand braucht um die Zählerstände zu ermitteln ist der Speedup direkt in der App spürbar.

Nach schneller gehts dann nur mit materialized view: 100-200ms.

andig commented 10 years ago

Das Thema können wir zu machen. Code ist fertig und kommt gemeinsam mit den Performanceverbesserungen. SQlite wird damit unterstützt: https://travis-ci.org/andig/volkszaehler.org/jobs/15754460