Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drouting: allow DB_BIGINT for dr_rules.ruleid column #3203

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

john08burke
Copy link
Contributor

Summary
Currently the drouting module only has support for DB_INT type for the dr_rules.ruleid column. This PR adds support for the DB_BIGINT type as well.

Details
I ran into this when trying to integrate the drouting module with some custom VIEW based schema that was using BIGINT for the ID column. The resulting logs are below and the table fails to load.

ERROR:drouting:dr_load_routing_info: column ruleid has a bad type [1], accepting only [0]

This should be a pretty safe one!

@bogdan-iancu
Copy link
Member

Shouldn't we extrapolate this to all tables with IDs? the dr_gateways and dr_carriers do also have a similar ID column.

@bogdan-iancu bogdan-iancu self-assigned this Oct 11, 2023
@john08burke
Copy link
Contributor Author

Yes good catch. I added DB_BIGINT support for those cases. On a side, should we be conditionally invoking VAL_BIGINT instead of VAL_INT? It seems to work, but I'm not sure how 😄... seems like in other modules we make the value extraction conditional like this.

@bogdan-iancu
Copy link
Member

@john08burke , your note points out a real issue actually. The DB_BIGINT is actually a long long , so a 64 bits data type. This may be incompatible with :

  • using VAL_INT() macro, so yes, we should either use VAL_BIGINT() (but it is an ugly trick, rely on the data alignment inside the union) or use a ternary operator to use the right macro
  • the final holder for the rule ID is an integer :-/ (for Carriers and GWs we have string ID which is used only for log printing and it is not stored into internal structures), so it may be subject to an overflow (while reading 64b from DB)

So, at least, we need to upgrade the rule->id to long long (and also check the implications of that - not sure where this ID is used further in the code).
And ideally come up with a 100% correct approach on DB_INT / DB_BIGINT issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants