nahanni / rw_redis_fdw

Other
97 stars 23 forks source link

Added support for valexpire column for set tabletype only. #16

Open deem0n opened 3 years ago

deem0n commented 3 years ago

Hello,

Thanks for your work, TTL handling is very useful. I just extended your code to support expiry for the individual set members. Probably it will be more fixes soon, after we run rw_redis_fdw with real data.

valexpire column allows to set expiration for the individual member of the set. It works similar to the expiry column. It uses EXPIREMEMBER command which is available only in KeyDB. It is not available in Redis as of 2020. https://docs.keydb.dev/docs/commands/#expiremember

Also fixed bug with error handling, so we really print error message instead of crashing in many cases.

Some more work is required, especially documentation.

l-d-x commented 3 years ago

Thanks for the patch. So this is specifically for KeyDB and not Redis? Also, why VALEXPIRE instead of its EXPIREMEMBER name?

On the outset, wouldn't it be more appropriate to have that done inside the TTL table with an optional member column?

I'll try to review in detail as soon as I can.

deem0n commented 3 years ago

Hi,

Yes, it works only with KeyDB. Redis has no EXPIREMEMBER command.

I used VALEXPIRE because I hope that someday Redis or KeyDB or something like Aerospike will support HKEY subkey expiration. So VALEXPIRE or EXPIREVAL is more generic name comparing to EXPIREMEMBER. Now I think EXPIREVAL is a better name, I hope we can use it!

The idea with TTL table can be implemented, but that way you need two operations to control EXPIRE for the set member. 1) you need to insert into 'set' table, 2) you need to assign expiration with the TTL table.

Currently, you can do it with one insert into 'set' table. Even FDW DEFAULT works, which is very handy:

CREATE FOREIGN TABLE portal.session_details (
    counter TEXT OPTIONS (redis 'key'), 
    member  TEXT,
    expire  INT OPTIONS (redis 'valexpire') DEFAULT 3600
    )
  SERVER redis_server
  OPTIONS (database '0', tabletype 'set');

So if you do INSERT INTO portal.session_details VALUES ('a','b') member b will be assigned 1 hour expiration time automatically.

Also, both valexpire and expiry can be used at the same time and working fine for 'set' table.

So I think we can add member for TTL table as you suggest, but I prefer to keep it for 'set' table as well.

deem0n commented 3 years ago

Also, it might be interesting to you, I tried to do the same idea with the original redis_fdw: https://github.com/luxms/redis_fdw/commit/bbed188f6d4b37b529b382ab1b16dd715845ac8a https://github.com/luxms/redis_fdw/blob/REL_11_STABLE/redis_fdw.c (Only this branch)

That way you set default expiration time with table OPTIONS: OPTIONS (database '0', tabletype 'set', keyexpire '3600').

The problem with original redis_fdw for me is that there is no way you can get collection size without iterating over collection. In other words there is no count(*) aggregate push down and it is very hard for me to implement it. So 'len' table from your project helped me.

l-d-x commented 3 years ago

I'll spend some time over the weekend to a look at your diffs.

Postgres v10+ supports aggregate pushdown for COUNT(*), but I haven't had time to study it yet and would likely need an example before any work can be done here so we can leverage it.