Optimize queries table

[DL6ER]

By submitting this pull request, I confirm the following:

• I have read and understood the contributors guide.
• I have checked that another pull request for this purpose does not exist.
• I have considered, and confirmed that this submission will be valuable to others.
• I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
• I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?:

10

---

This PR optimizes the queries table by using link-tables with IDs instead of storing the domain, client and forward as strings over and over. This is not a breaking change to users as we provide a VIEW queries in place of the old TABLE queries that will give the same results for external queries as before:

VIEW queries

$ sqlite3 /etc/pihole/pihole-FTL.db -header "SELECT * FROM queries ORDER BY id DESC LIMIT 1;"                                                                                           
id|timestamp|type|status|domain|client|forward|additional_info
19391970|1639654453|2|2|forum.arduino.cc|192.168.2.206|127.0.0.1#5353|

TABLE query_storage (holding the real data)

$ sqlite3 /etc/pihole/pihole-FTL.db -header "SELECT * FROM query_storage ORDER BY id DESC LIMIT 1;"                                                                                           
id|timestamp|type|status|domain|client|forward|additional_info
19391970|1639654453|2|2|114|2|1|

In the example above, storage required per query before:

   4    sizeof(id)
+  4    sizeof(timestamp)
+  1    sizeof(type)
+  1    sizeof(status)
+ 16    strlen("forum.arduino.cc") + 1
+ 14    strlen("192.168.2.206") + 1
+ 15    strlen("127.0.0.1#5353") + 1
+  1    sizeof(NULL)
= 56 bytes per record

Storage required per query with the optimization of this PR:

   4    sizeof(id)
+  4    sizeof(timestamp)
+  1    sizeof(type)
+  1    sizeof(status)
+  1    sizeof(domain)
+  1    sizeof(client)
+  1    sizeof(forward)
+  1    sizeof(additional_info)
= 14 bytes per record

This reduction by factor 4 easily outperforms the tiny extra amount of space we need for the newly introduced link-tables. We chose this hybrid approach through the VIEW because we cannot afford rewriting the entire queries table as this would literally take forever on low-end devices such as Raspberry Pis from the early days.

This also lays the foundation for a requested feature that is availability of host names in the long-term database as the linked client ID are in fact a pair of (ip,hostname) in the database. This allows us to store both without any really noticeable overhead.

This PR comes with a tiny drawback (that isn't really one, though): After the database has been optimized (upgraded to version 11), older versions of FTL will not be able to write new queries to the database. It is possible to revert the upgrade of the database without data-loss, however, it is a multiple-step task and should not be done copy-paste-like. That's why I'm not giving the precise instructions to do this here.

edit We furthermore save some space on the additional_information field that con contain domains when blocking during deep CNAME inspection.

[DL6ER]

Putting this back to draft mode as we might want to explore another link-table for additional_info

[DL6ER]

Ready for review. The additional_info column is now also linked to a corresponding table giving us the flexibility to add multiple properties to a query in the future.

[DL6ER]

Branch updated from latest development

[yubiuser]

After switching to that branch

[2022-01-09 21:00:00.718 2828/T2832] SQLite3 message: cannot modify queries because it is a view in "DELETE FROM queries WHERE timestamp <= 1633982400" (1)
[2022-01-09 21:00:00.719 2828/T2832] ERROR: SQL query "DELETE FROM queries WHERE timestamp <= 1633982400" failed: SQL logic error
[2022-01-09 21:00:00.719 2828/T2832] delete_old_queries_in_DB(): Deleting queries due to age of entries failed

[yubiuser]

This will need some documentation changes in https://docs.pi-hole.net/database/ftl/

E.g. addinfo_by_id meaning of type

[DL6ER]

@yubiuser Thanks for the error message, you indeed found the spot where the renaming queries -> query_storage is missing. I pushed an update.

[yubiuser]

Error is fixed
[2022-01-10 07:00:00.352 24148/T24152] Notice: Database size is 149.30 MB, deleted 2955 rows

[yubiuser]

Please remember the documentation.

Thanks for that update.

Do we need any modification on web? I know it will use the queries view now automatically, but generating the view can take some time. We might not need all columns from there, e.g. wenn computing "Long term graph"

[yubiuser]

Just an idea: now that you shrank the size per record tremendously, should we save the reply as well?

[DL6ER]

I will use this PR as reminder by only merging when I had time to work on the documentation changes.

Do we need any modification on web? I know it will use the queries view now automatically, but generating the view can take some time. We might not need all columns from there, e.g. wenn computing "Long term graph"

We are not requesting these columns through the VIEW, hence, they are also not looked up. For instance, take https://github.com/pi-hole/AdminLTE/blob/682a174330d37c4bb8222ff371a3a4b7d363fafa/api_db.php#L264

SELECT COUNT(timestamp) FROM queries;

compiles to the following database engine bytecode:

addr  opcode         p1    p2    p3    p4             p5  comment      
----  -------------  ----  ----  ----  -------------  --  -------------
0     Init           0     11    0                    00  Start at 11  
1     Null           0     1     2                    00  r[1..2]=NULL 
2     OpenRead       1     2     0     8              00  root=2 iDb=0; query_storage
3     Rewind         1     7     0                    00               
4       Column         1     1     3                    00  r[3]=query_storage.timestamp
5       AggStep        0     3     1     count(1)       01  accum=r[1] step(r[3])
6     Next           1     4     0                    01               
7     AggFinal       1     1     0     count(1)       00  accum=r[1] N=1
8     Copy           1     4     0                    00  r[4]=r[1]    
9     ResultRow      4     1     0                    00  output=r[4]  
10    Halt           0     0     0                    00               
11    Transaction    0     0     22    0              01  usesStmtJournal=0
12    Goto           0     1     0                    00

Querying query_storage does not change anything:

SELECT COUNT(timestamp) FROM query_storage;

addr  opcode         p1    p2    p3    p4             p5  comment      
----  -------------  ----  ----  ----  -------------  --  -------------
0     Init           0     11    0                    00  Start at 11  
1     Null           0     1     2                    00  r[1..2]=NULL 
2     OpenRead       1     4     0     k(2,,)         00  root=4 iDb=0; idx_queries_timestamps
3     Rewind         1     7     3     0              00               
4       Column         1     0     3                    00  r[3]=query_storage.timestamp
5       AggStep        0     3     1     count(1)       01  accum=r[1] step(r[3])
6     Next           1     4     0                    01               
7     AggFinal       1     1     0     count(1)       00  accum=r[1] N=1
8     Copy           1     4     0                    00  r[4]=r[1]    
9     ResultRow      4     1     0                    00  output=r[4]  
10    Halt           0     0     0                    00               
11    Transaction    0     0     22    0              01  usesStmtJournal=0
12    Goto           0     1     0                    00

as the former already uses the real storage. The same holds true for all other queries.

Just an idea: now that you shrank the size per record tremendously, should we save the reply as well?

For v6.0, I already implemented storing everything visible on the Query Log (also reply time, and DNSSEC status). We can do this already now, that's a good idea. Something for a follow-up PR.

[pralor-bot]

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-14-web-v5-11-and-core-v5-9-released/53529/1

[pralor-bot]

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/delete-queries-error/53761/2

[pralor-bot]

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/adding-many-lists-via-shell/54356/2

[pralor-bot]

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-blocking-less-than-expected/54394/8