Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Drop chunks from materialized hypertables #1666
Drop chunks from materialized hypertables #1666
Changes from all commits
c77780b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Just nit, I guess those two cases can be combined together to avoid having error message duplication, like this:
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.
Just noticed that sometimes we are using
elog(ERROR)
and sometimesereport(ERROR)
here, maybe we should stick to one of them, unless there is a good reason for itThere 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.
Add newline
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.
Maybe just do
Assert(htid == INVALID_HYPERTABLE_ID)
instead of count when assigninghtid
. In case if Assert is disabled, we might get compiler warning saying that count is set but not used.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 suggest a different error code, since this one is reserved for invalid parameter values when calling (user-facing) SQL functions. For instance, user calls
SELECT set_port(port => 134000);
then the value forport
is invalid (outside the port range).Also, in my understanding, having a NULL integer now func is valid.
I suggest INTERNAL_ERROR or just a simple
elog
.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.
this code path is exercised by add drop_chunks policy and compress_chunks policy. So this is a user facing error.
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.
My point is that
integer_now_func
andinteger_now_func_schema
aren't, AFAIK, set as part of those functions, like in myset_port
example. Further, the error is not about a function parameter but about some values of internal metadata being NULL.So, I am not sure it makes sense to raise
ERRCODE_INVALID_PARAMETER_VALUE
. The error should tell you that the system is in the wrong state for the given operation.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.
These are errors for the "older_than" parameter for the policy. There is a specific action needed on the user's part to fix this issue. older_than parameter on integer-time based hypertables doesn't make sense without specifying a now() function. INTERNAL_ERROR to me typically signifies something unexpected happening in the code which is probably a bug.
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.
Do we still need to put blank line after a variable definition?