Skip to content

Commit

Permalink
db_redis: allow deletion of all rows
Browse files Browse the repository at this point in the history
  • Loading branch information
@rdboisvert @henningw
rdboisvert authored and henningw committed Nov 20, 2019
1 parent a8cc28b commit bf2ecd4
Showing 1 changed file with 7 additions and 0 deletions.
7 changes: 7 additions & 0 deletions src/modules/db_redis/redis_dbase.c
View file
Expand Up @@ -1325,6 +1325,12 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con
if (tmp)
db_redis_key_free(&tmp);

// skip if delete all rows
if (!*manual_keys_count) {
db_redis_key_free (&query_v);
goto skipkeys;
}

if (db_redis_key_prepend_string(&query_v, "HMGET", 5) != 0) {
LM_ERR("Failed to set hmget command to pre-delete query\n");
goto error;
Expand Down Expand Up @@ -1416,6 +1422,7 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con
db_vals = NULL;
db_redis_free_reply(&reply);

skipkeys:
if (db_redis_key_add_string(&query_v, "DEL", 3) != 0) {
LM_ERR("Failed to add del command to delete query\n");
goto error;
Expand Down

14 comments on commit bf2ecd4

@rfuchs
Copy link
Member

@rfuchs rfuchs commented on bf2ecd4 Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @rdboisvert and @henningw

We bumped into this commit on our deployments because it was causing some issues. In particular it makes the Redis SETs that we use a pseudo-indexes being left over in the Redis DB after a delete operation. The reason is that even for deletes that can be done directly (no/empty "manual keys") the contents of the entry still need to be fetched because the contents are part of the key names used for pseudo-index SETs.

The patch we came up with is exactly the reversal of this commit, so obviously I need to ask about the intentions behind it, since I'm sure you had a good reason to do it :)

@miconda
Copy link
Member

@miconda miconda commented on bf2ecd4 Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rfuchs - apparently the patch was supposed to delete all entries, without a need to match on filters, according to PR #2140. Like the sql delete from table without where clause.

Now I understand that the sets keeping indexes are left. Does it happen for every type of delete, even when it has filters (i.e., where conditions)? Or only in the case of delete all?

@rfuchs
Copy link
Member

@rfuchs rfuchs commented on bf2ecd4 Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miconda it happens on every delete. Our "table definitions" use Redis sets to simulate non-unique database indexes. For example if we have a key entry:foobar that is a hash and includes a key "test = blah" (so, HMSET entry:foobar test blah) and we use the "test" key as a non-unique index, then we'd have a set index:test:blah that includes entry:foobar as member (SADD index:test:blah entry:foobar). So when deleting entry:foobar we need to delete that from the index:test:blah set (SREM ...). This requires fetching the contents of the entry:foobar hash when deleting it. So with this commit, when entry:foobar is deleted directly by name, there will be no "manual keys" and so the HMGET is skipped and so the SREM fails.

@miconda
Copy link
Member

@miconda miconda commented on bf2ecd4 Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If @rdboisvert or @henningw don't have other comments, it can be reverted, or maybe a modparam should added to enable/disable the behaviour of this patch, with default setting to be disabled, as it seems to be the one to have it working.

As you said, it would be good to know what was the reason for this patch, what cases it was supposed to fix. I merged it based on the fact that we have to support the delete all operation and seemed to have that purpose.

@henningw
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a review and merged it, if it causes a regression I will revert it. But @rdboisvert can problably best comment on the reasoning for this change, wait a bit for him to reply.

@rdboisvert
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would like to have a delete row function as described in in section 9.2.9 of the Kamailio SIP Server v3.2.0 Development Guide. If I implemented the feature incorrectly then we would appreciate someone fixing this.

Thanks!

@rfuchs
Copy link
Member

@rfuchs rfuchs commented on bf2ecd4 Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdboisvert it should already do this. Which part was not working for you?

@henningw
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the original issue "Former code always assumed filters would be involved so this change skips the filter matching if no filter is applied."

@rfuchs
Copy link
Member

@rfuchs rfuchs commented on bf2ecd4 Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but did it actually break the delete operation, or was it just a performance optimisation? In either case I can probably come up with an alternative patch that doesn't break the set indexes.

@henningw
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rfuchs I think this sounds more like a bug in the patch, not taking in account your mode of operation. If you can create a patch that fix to the original problem that does not break your operation, this would be great.

@rdboisvert
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for a lack of clarity in my response. What I tried to patch is the bug that it did not delete all rows, or as expressed clearly by @miconda and @henningw, a unfiltered delete of all rows.

@rfuchs
Copy link
Member

@rfuchs rfuchs commented on bf2ecd4 Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdboisvert I would propose cbc4d1e to fix this. It should allow for full table deletion while also fixing our use case. Could you confirm that this also works for you?

@rdboisvert
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code I'm not sure I see how it can work but I will build a test environment and let you know. It will probably take me a week to get this together.

@rdboisvert
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rfuchs I was able to test this today and it now works as expected. Thanks for the correction.

Please sign in to comment.