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

Improve error message when adding data nodes #3494

Merged
merged 1 commit into from
Aug 19, 2021

Conversation

mkindahl
Copy link
Contributor

When data nodes are added but are missing USAGE privileges, a hint will
be shown suggesting to add more data nodes. This is misleading since
data nodes are added. Instead, if there are data nodes but they are not
used, the hint will be to fix the privileges for the data nodes.

Fixes #3479

@codecov
Copy link

codecov bot commented Aug 17, 2021

Codecov Report

Merging #3494 (c63c8a1) into master (36a82d0) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3494      +/-   ##
==========================================
- Coverage   90.71%   90.70%   -0.02%     
==========================================
  Files         212      212              
  Lines       36266    36268       +2     
==========================================
- Hits        32898    32896       -2     
- Misses       3368     3372       +4     
Impacted Files Coverage Δ
tsl/src/hypertable.c 98.86% <100.00%> (+0.02%) ⬆️
src/loader/bgw_message_queue.c 85.52% <0.00%> (-2.64%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36a82d0...c63c8a1. Read the comment docs.

@mkindahl mkindahl marked this pull request as ready for review August 17, 2021 18:58
@mkindahl mkindahl requested a review from a team as a code owner August 17, 2021 18:58
@mkindahl mkindahl requested review from pmwkaa, afiskon and nikkhils and removed request for a team August 17, 2021 18:58
@@ -164,7 +165,12 @@ hypertable_get_and_validate_data_nodes(ArrayType *nodearr)
ereport(ERROR,
(errcode(ERRCODE_TS_INSUFFICIENT_NUM_DATA_NODES),
errmsg("no data nodes can be assigned to the hypertable"),
errhint("Add data nodes to the database.")));
errdetail(nodearr == NULL && list_length(all_data_nodes) == 0 ?
Copy link
Contributor

@mfundul mfundul Aug 18, 2021

Choose a reason for hiding this comment

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

Small nitpick, in the code there are 3 consecutive uses of list_length(all_data_nodes) and 2 consecutive uses of (nodearr == NULL && list_length(all_data_nodes) == 0)

It would be easier to read if it was something like:

int num_all_data_nodes = list_length(all_data_nodes);
bool permission_error = !(nodearr == NULL && num_all_data_nodes == 0);

The same thing happens in line 153 where we use list_length(data_nodes); when we already have num_data_nodes set. We could fix that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think that num_all_data_nodes is easier to read than list_length(all_data_nodes), they are pretty much equivalent.

@mkindahl mkindahl self-assigned this Aug 18, 2021
NOTICE: 3 of 3 data nodes not used by this hypertable due to lack of permissions
HINT: Grant USAGE on data nodes to attach them to a hypertable.
ERROR: no data nodes can be assigned to the hypertable
DETAIL: Data nodes existed, but none had USAGE privilege.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if "Data nodes exist, but none have USAGE privilege" is better than the above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that is better. Fixed.

Copy link
Contributor

@nikkhils nikkhils left a comment

Choose a reason for hiding this comment

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

LGTM. Apart from the one comment about which sentence might be better.

When data nodes are added but are missing USAGE privileges, a hint will
be shown suggesting to add more data nodes. This is misleading since
data nodes are added. Instead, if there are data nodes but they are not
used, the hint will be to fix the privileges for the data nodes.

Fixes timescale#3479
@mkindahl mkindahl enabled auto-merge (rebase) August 19, 2021 06:24
@mkindahl mkindahl merged commit c4eb9b6 into timescale:master Aug 19, 2021
@mkindahl mkindahl deleted the fix_error_message branch August 19, 2021 11:50
mkindahl added a commit to mkindahl/timescaledb that referenced this pull request Aug 19, 2021
This release contains bug fixes since the 2.4.0 release. We deem it
high priority to upgrade since it is needed to support PostgreSQL 12.8
and 13.4.

**Bugfixes**
* timescale#3430 Fix havingqual processing for continuous aggregates
* timescale#3468 Disable tests by default if tools are not found
* timescale#3468 Fix crash while tracking alter table commands
* timescale#3494 Improve error message when adding data nodes
* timescale#3498 Fix continuous agg bgw job failure for PG 12.8 and 13.4

**Thanks**
* @brianbenns for reporting a segfault with continuous aggregates
mkindahl added a commit to mkindahl/timescaledb that referenced this pull request Aug 19, 2021
This release contains bug fixes since the 2.4.0 release. We deem it
high priority to upgrade since it is needed to support PostgreSQL 12.8
and 13.4.

**Bugfixes**
* timescale#3430 Fix havingqual processing for continuous aggregates
* timescale#3468 Disable tests by default if tools are not found
* timescale#3468 Fix crash while tracking alter table commands
* timescale#3494 Improve error message when adding data nodes
* timescale#3498 Fix continuous agg bgw job failure for PG 12.8 and 13.4

**Thanks**
* @brianbenns for reporting a segfault with continuous aggregates
mkindahl added a commit to mkindahl/timescaledb that referenced this pull request Aug 19, 2021
This release contains bug fixes since the 2.4.0 release. We deem it
high priority to upgrade since it is needed to support PostgreSQL 12.8
and 13.4.

**Bugfixes**
* timescale#3430 Fix havingqual processing for continuous aggregates
* timescale#3468 Disable tests by default if tools are not found
* timescale#3468 Fix crash while tracking alter table commands
* timescale#3494 Improve error message when adding data nodes
* timescale#3498 Fix continuous agg bgw job failure for PG 12.8 and 13.4

**Thanks**
* @brianbenns for reporting a segfault with continuous aggregates
mkindahl added a commit to mkindahl/timescaledb that referenced this pull request Aug 19, 2021
This release contains bug fixes since the 2.4.0 release. We deem it
high priority to upgrade since it is needed to support PostgreSQL 12.8
and 13.4.

**Bugfixes**
* timescale#3430 Fix havingqual processing for continuous aggregates
* timescale#3468 Disable tests by default if tools are not found
* timescale#3468 Fix crash while tracking alter table commands
* timescale#3494 Improve error message when adding data nodes
* timescale#3498 Fix continuous agg bgw job failure for PG 12.8 and 13.4

**Thanks**
* @brianbenns for reporting a segfault with continuous aggregates
mkindahl added a commit to mkindahl/timescaledb that referenced this pull request Aug 19, 2021
This release contains bug fixes since the 2.4.0 release. We deem it
high priority to upgrade since it is needed to support PostgreSQL 12.8
and 13.4.

**Bugfixes**
* timescale#3430 Fix havingqual processing for continuous aggregates
* timescale#3468 Disable tests by default if tools are not found
* timescale#3468 Fix crash while tracking alter table commands
* timescale#3494 Improve error message when adding data nodes
* timescale#3498 Fix continuous agg bgw job failure for PG 12.8 and 13.4

**Thanks**
* @brianbenns for reporting a segfault with continuous aggregates
mkindahl added a commit to mkindahl/timescaledb that referenced this pull request Aug 19, 2021
This release contains bug fixes since the 2.4.0 release. We deem it
high priority to upgrade since it is needed to support PostgreSQL 12.8
and 13.4.

**Bugfixes**
* timescale#3430 Fix havingqual processing for continuous aggregates
* timescale#3468 Disable tests by default if tools are not found
* timescale#3468 Fix crash while tracking alter table commands
* timescale#3494 Improve error message when adding data nodes
* timescale#3498 Fix continuous agg bgw job failure for PG 12.8 and 13.4

**Thanks**
* @brianbenns for reporting a segfault with continuous aggregates
mkindahl added a commit to mkindahl/timescaledb that referenced this pull request Aug 19, 2021
This release contains bug fixes since the 2.4.0 release. We deem it
high priority to upgrade since it is needed to support PostgreSQL 12.8
and 13.4.

**Bugfixes**
* timescale#3430 Fix havingqual processing for continuous aggregates
* timescale#3468 Disable tests by default if tools are not found
* timescale#3468 Fix crash while tracking alter table commands
* timescale#3494 Improve error message when adding data nodes
* timescale#3498 Fix continuous agg bgw job failure for PG 12.8 and 13.4

**Thanks**
* @brianbenns for reporting a segfault with continuous aggregates
mkindahl added a commit to mkindahl/timescaledb that referenced this pull request Aug 19, 2021
This release contains bug fixes since the 2.4.0 release. We deem it
high priority to upgrade since it is needed to support PostgreSQL 12.8
and 13.4.

**Bugfixes**
* timescale#3430 Fix havingqual processing for continuous aggregates
* timescale#3468 Disable tests by default if tools are not found
* timescale#3468 Fix crash while tracking alter table commands
* timescale#3494 Improve error message when adding data nodes
* timescale#3498 Fix continuous agg bgw job failure for PG 12.8 and 13.4

**Thanks**
* @brianbenns for reporting a segfault with continuous aggregates
mkindahl added a commit to mkindahl/timescaledb that referenced this pull request Aug 19, 2021
This release contains bug fixes since the 2.4.0 release. We deem it
high priority to upgrade since it is needed to support PostgreSQL 12.8
and 13.4.

**Bugfixes**
* timescale#3430 Fix havingqual processing for continuous aggregates
* timescale#3468 Disable tests by default if tools are not found
* timescale#3468 Fix crash while tracking alter table commands
* timescale#3494 Improve error message when adding data nodes
* timescale#3498 Fix continuous agg bgw job failure for PG 12.8 and 13.4

**Thanks**
* @brianbenns for reporting a segfault with continuous aggregates
mkindahl added a commit to mkindahl/timescaledb that referenced this pull request Aug 19, 2021
This release contains bug fixes since the 2.4.0 release.  We deem it
high priority to upgrade.

The release fixes continous aggregate refresh for postgres 12.8 and
13.4, a crash with ALTER TABLE commands and a crash with continuous
aggregates with HAVING clause.

**Bugfixes**
* timescale#3430 Fix havingqual processing for continuous aggregates
* timescale#3468 Disable tests by default if tools are not found
* timescale#3462 Fix crash while tracking alter table commands
* timescale#3489 Fix continuous agg bgw job failure for PG 12.8 and 13.4
* timescale#3494 Improve error message when adding data nodes

**Thanks**
* @brianbenns for reporting a segfault with continuous aggregates
* @brianbenns for reporting a segfault with continuous aggregates
* @usego for reporting an issue with continuous aggregate refresh on PG 13.4
mkindahl added a commit that referenced this pull request Aug 19, 2021
This release contains bug fixes since the 2.4.0 release.  We deem it
high priority to upgrade.

The release fixes continous aggregate refresh for postgres 12.8 and
13.4, a crash with ALTER TABLE commands and a crash with continuous
aggregates with HAVING clause.

**Bugfixes**
* #3430 Fix havingqual processing for continuous aggregates
* #3468 Disable tests by default if tools are not found
* #3462 Fix crash while tracking alter table commands
* #3489 Fix continuous agg bgw job failure for PG 12.8 and 13.4
* #3494 Improve error message when adding data nodes

**Thanks**
* @brianbenns for reporting a segfault with continuous aggregates
* @brianbenns for reporting a segfault with continuous aggregates
* @usego for reporting an issue with continuous aggregate refresh on PG 13.4
mkindahl added a commit to mkindahl/timescaledb that referenced this pull request Aug 19, 2021
This release contains bug fixes since the 2.4.0 release.  We deem it
high priority to upgrade.

The release fixes continous aggregate refresh for postgres 12.8 and
13.4, a crash with ALTER TABLE commands and a crash with continuous
aggregates with HAVING clause.

**Bugfixes**
* timescale#3430 Fix havingqual processing for continuous aggregates
* timescale#3468 Disable tests by default if tools are not found
* timescale#3462 Fix crash while tracking alter table commands
* timescale#3489 Fix continuous agg bgw job failure for PG 12.8 and 13.4
* timescale#3494 Improve error message when adding data nodes

**Thanks**
* @brianbenns for reporting a segfault with continuous aggregates
* @brianbenns for reporting a segfault with continuous aggregates
* @usego for reporting an issue with continuous aggregate refresh on PG 13.4
mkindahl added a commit that referenced this pull request Aug 19, 2021
This release contains bug fixes since the 2.4.0 release.  We deem it
high priority to upgrade.

The release fixes continous aggregate refresh for postgres 12.8 and
13.4, a crash with ALTER TABLE commands and a crash with continuous
aggregates with HAVING clause.

**Bugfixes**
* #3430 Fix havingqual processing for continuous aggregates
* #3468 Disable tests by default if tools are not found
* #3462 Fix crash while tracking alter table commands
* #3489 Fix continuous agg bgw job failure for PG 12.8 and 13.4
* #3494 Improve error message when adding data nodes

**Thanks**
* @brianbenns for reporting a segfault with continuous aggregates
* @usego for reporting an issue with continuous aggregate refresh on PG 13.4
mkindahl added a commit that referenced this pull request Aug 19, 2021
This release contains bug fixes since the 2.4.0 release.  We deem it
high priority to upgrade.

The release fixes continous aggregate refresh for postgres 12.8 and
13.4, a crash with ALTER TABLE commands and a crash with continuous
aggregates with HAVING clause.

**Bugfixes**
* #3430 Fix havingqual processing for continuous aggregates
* #3468 Disable tests by default if tools are not found
* #3462 Fix crash while tracking alter table commands
* #3489 Fix continuous agg bgw job failure for PG 12.8 and 13.4
* #3494 Improve error message when adding data nodes

**Thanks**
* @brianbenns for reporting a segfault with continuous aggregates
* @brianbenns for reporting a segfault with continuous aggregates
* @usego for reporting an issue with continuous aggregate refresh on PG 13.4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants