-
Notifications
You must be signed in to change notification settings - Fork 662
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
Fix error in master_disable_node/citus_disable_node #7492
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #7492 +/- ##
==========================================
- Coverage 89.60% 89.52% -0.08%
==========================================
Files 282 282
Lines 60370 60368 -2
Branches 7521 7522 +1
==========================================
- Hits 54092 54043 -49
- Misses 4124 4165 +41
- Partials 2154 2160 +6 |
@@ -507,7 +507,13 @@ citus_disable_node(PG_FUNCTION_ARGS) | |||
{ | |||
text *nodeNameText = PG_GETARG_TEXT_P(0); | |||
int32 nodePort = PG_GETARG_INT32(1); | |||
bool synchronousDisableNode = PG_GETARG_BOOL(2); | |||
|
|||
bool synchronousDisableNode = 1; |
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 don't understand how master_disable_node() should work (see also #2063 on this topic), but if synchronousDisableNode = 0 by default, some tests are failing. For example, isolation_add_remove_node with
step s1-disable-node-1:
SELECT 1 FROM master_disable_node('localhost', 57637);
SELECT public.wait_until_metadata_sync();
-?column?
----------------------------------------------------------------------
- 1
-(1 row)
-
-wait_until_metadata_sync
----------------------------------------------------------------------
-
-(1 row)
-
+ERROR: disabling the first worker node in the metadata is not allowed
step s2-disable-node-1:
SELECT 1 FROM master_disable_node('localhost', 57637);
SELECT public.wait_until_metadata_sync();
- <waiting ...>
+
+ERROR: disabling the first worker node in the metadata is not allowed
in diff
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.
master_disable_node
is deprecated since Citus 10.0. So it's behaviour is normally not really intended to change. I think 549edca inadvertently did change the behaviour of this function. Since no-one complained since then, I agree that it makes sense to make continue passing this test.
@@ -507,7 +507,13 @@ citus_disable_node(PG_FUNCTION_ARGS) | |||
{ | |||
text *nodeNameText = PG_GETARG_TEXT_P(0); | |||
int32 nodePort = PG_GETARG_INT32(1); | |||
bool synchronousDisableNode = PG_GETARG_BOOL(2); | |||
|
|||
bool synchronousDisableNode = 1; |
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.
master_disable_node
is deprecated since Citus 10.0. So it's behaviour is normally not really intended to change. I think 549edca inadvertently did change the behaviour of this function. Since no-one complained since then, I agree that it makes sense to make continue passing this test.
This fixes #7454: master_disable_node() has only two arguments, but calls citus_disable_node() that tries to read three arguments Co-authored-by: Karina Litskevich <litskevichkarina@gmail.com>
DESCRIPTION: Fixes undefined behavior in master_disable_node due to argument mismatch
This fixes #7454: master_disable_node() has only two arguments, but calls citus_disable_node() that tries to read three arguments