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.

Sign up for GitHub

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

Jump to bottom

Fix uninitialized bucket_info.htpartcolno warning #5271

Merged
merged 1 commit into from
Feb 17, 2023
Merged

Fix uninitialized bucket_info.htpartcolno warning #5271

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 8 additions & 8 deletions tsl/src/continuous_aggs/create.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1487,6 +1487,13 @@ cagg_validate_query(const Query *query, const bool finalized, const char *cagg_s
}

ts_cache_release(hcache);

/*
* We need a GROUP By clause with time_bucket on the partitioning
* column of the hypertable
*/
Assert(query->groupClause);
caggtimebucket_validate(&bucket_info, query->groupClause, query->targetList);
Comment on lines +1491 to +1496
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're moving the code into an if-statement, it is a possibility that not setting query->groupClause query->targetList could cause issues later in the execution. I am not sure if that can be triggered in a reasonable way, but could you investigate this?

}

/* Check row security settings for the table. */
Expand All @@ -1495,14 +1502,7 @@ cagg_validate_query(const Query *query, const bool finalized, const char *cagg_s
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot create continuous aggregate on hypertable with row security")));

/*
* We need a GROUP By clause with time_bucket on the partitioning
* column of the hypertable.
*/
Assert(query->groupClause);
caggtimebucket_validate(&bucket_info, query->groupClause, query->targetList);

/* Nested cagg validations. */
/* nested cagg validations */
if (is_nested)
Comment on lines -1498 to 1506
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. trying to sort this out a little more, to see if anything could go wrong.

The function caggtimebucket_validate will update the bucket_width and some other fields in bucket_info if it is executed. These are used in the code inside the if (is_nested) that follows here. The default value for bucket_info.bucket_width is -1, which is an invalid value.

The variable is_nested is set to true if and only if rte->relkind == RELKIND_VIEW in the code block above, but since we call the code at the end of the if-statement, it seems like the code path bucket_width is used below this code also contains a call to the function caggtimebucket_validate.

The only situation where bucket_width and friends can be unmodified is then when it is not a relation or a view. In this case, the bucket info is returned to the calling function, which is then used in a call to build_union_query, but this function does not seem to use this information, so tentatively it seems like the change is safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that is weird is that the field hpartcolno is used inside build_union_query but this do not trigger an error even though the code in caggtimebucket_validate does it.

{
int64 bucket_width = 0, bucket_width_parent = 0;
Expand Down