-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
add: per request statement timeout using prefer header #4584
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
base: main
Are you sure you want to change the base?
Conversation
d074eb9 to
2c5589f
Compare
5f1d601 to
6963e9a
Compare
|
We should also fail in case a value exceeds the per role timeout. Otherwise it's abusable. This failure should happen at the plan level, before hitting the db. I think it will need a new error code. |
a82c07f to
0d6370e
Compare
| -- It is safe to use fromJust because the string being read is guaranteed | ||
| -- to be number after striping the unit. | ||
| stripUnitAndGetInt unit val = fromJust $ BS.stripSuffix unit val >>= readMaybe | ||
| rtValueToSeconds :: ByteString -> Int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by all of this logic, why do we need to parse a time unit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also fail in case a value exceeds the per role timeout. Otherwise it's abusable
I'm confused by all of this logic, why do we need to parse a time unit?
That's because the statement_timeout value of the role could be like 1min or 100ms depending upon the role. So, we need to throw an error depending on whether the timeout exceeds role setting or not and for that, we must parse the value, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do the conversion in postgres to avoid the parsing? For example with:
select extract(epoch from current_setting('statement_timeout', false)::interval)::int;That should give you the number of seconds regardless of the input format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great.
However, if we do this conversion at the time of querying the role settings, that would change the original role settings, which we explicitly add to pre-query (BTW, I'm not sure why we do this, wouldn't postgres already know the settings?) here:
postgrest/src/PostgREST/Query/PreQuery.hs
Line 51 in 8f5fe3f
| roleSettingsSql = setConfigWithDynamicName <$> HM.toList (fromMaybe mempty $ HM.lookup authRole configRoleSettings) |
So, we would then have to keep the two sets of role settings, one with the conversion applied on and the original ones without the conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we explicitly add to pre-query (BTW, I'm not sure why we do this, wouldn't postgres already know the settings?)
The settings set via ALTER ROLE ... only apply when logging in with that role. PostgreSQL does not apply them when doing a SET ROLE. That means, these role-specific settings are only set when we do it manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@taimoorzaeem Hm, so we can't avoid this logic with the above snippet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. If we try to avoid this logic using the above snippet, we would end with maintaining more state (keep extra pre-computed values, even when they are never used) so I think we should avoid it.
0d6370e to
6481e8e
Compare
6481e8e to
a37d497
Compare
Adds a feature to set `statement_timeout` using `Prefer: timeout` header. This also introduces a `PGRST129` error which is returned when the timeout preferred exceeds the per-role timeout, which prevents misuse of this feature. Signed-off-by: Taimoor Zaeem <[email protected]>
a37d497 to
1998a61
Compare
Adds a feature to set
statement_timeoutusingPrefer: timeoutheader. This also introduces aPGRST129error which is returned when the timeout preferred exceeds the per-role timeout, which prevents misuse of this feature.Closes #4381.