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

Citus Fix json_build_object with tables with more than 50 columns #10229

Closed

Conversation

ammirator-administrator
Copy link

@ammirator-administrator ammirator-administrator commented May 8, 2024

Postgres has by default a limitation for functions names max_function_args and is set to 100, which means you can't pass more that 100 arguments to a pg function otherwise it throws error

BTW I'm talking about postgres with Citus, seems that in postgres implementation you guys use row_json which is fine and that;s why there it doesn't happen

The implementation in hasura is that for every table column it adds 2 arguments in json_build_object object one lets say "a" and another "root.a"
WHICH means that with hasura this limitation becomes 50 column maximum per table, which is not really friendly

To change max_function_args in Postgres you just have to recompile Postgres, which is something I would not sugest to someone to do) even Postgres team doesn't sugest to compile your own version

SInce postgres way is not a solution It needs to be fixed in Hasura definitelly because Postgres limitation is good 100 but because of how hasura adds args it just becomes 50 which is already a problem for most apps, I have a small table where I keep app config, and is normal that a config would have more that 50 columns but hasura doesn't allow to query this table at all because of this limitation

PS. I just opened this PR because I see there are 2k issues and the chance that the issue will be read soon is zero
But I consider this a serious limitation
SInce I do not know haskel I cant contribute so I would love some help from Hasura team to implement this
I think this fix for them would take not much time since they know haskel and the fix most probably is already described here
https://postgrespro.com/list/thread-id/2486891

@ammirator-administrator ammirator-administrator requested a review from a team as a code owner May 8, 2024 21:49
@hasura-bot
Copy link
Contributor

🤖

Thanks for your PR @ammirator-administrator!

A member of our team will review it and provide feedback shortly. We appreciate your patience and enthusiasm in improving Hasura.

Thank you for your contribution 🚀

@CLAassistant
Copy link

CLAassistant commented May 8, 2024

CLA assistant check
All committers have signed the CLA.

@ammirator-administrator ammirator-administrator changed the title Fix json_build_object with tables with more than 50 columns Citus Fix json_build_object with tables with more than 50 columns May 8, 2024
@ammirator-administrator
Copy link
Author

Also I do not understand why in postgres driverr/addaper you guys use row_json which is good but with citus addapter you use json_build_object
Citus is fully compatible with postgres since is just a postgre sextension, would it be correct to use same methods? this way we awoid these limitations
I guess in all adapters where you guys use json_build_object we should have this limitation of maximum 100 args because thats the default for postgres and cant be changed

@ammirator-administrator
Copy link
Author

If i use in my databases.yaml
this

kind: postgres

instead of

kind: citus

Then everything works, but I just simply do not know if that is ok and what I lose by doing so

@gherciu
Copy link

gherciu commented May 9, 2024

+1 This is supper needed and is a limitation from Hasura

Big companies do not use just postgres since it doesn't make sense in big apps but with some extension like Citus
And if the Hasura Citus adapter implementation has these limitations it is sad for big companies which may try to us eHasura

@SamirTalwar
Copy link
Contributor

This is not a change request but a feature request, already tracked in #10227. While it is important, I suggest we have the discussion and talk about fixing it in one place, not two, so as not to split the conversation.

@hasura-bot
Copy link
Contributor

🤖

Hi, @ammirator-administrator

We regret to inform you that your PR has not been merged. However, your efforts are greatly appreciated, and we encourage you to stay involved.

Please consider contributing to other open issues that might align with your expertise. Our community is active on Discord, and we're here to support you with any questions you might have.

Thank you for your understanding and continued support.

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.

5 participants