fix bug #8470 citus_activate_node with secondary nodes is not activate

[Alexandre (akalend)]

DESCRIPTION: bug #8470 The citus_activate_node() does not activate secondary nodes. This action is necessary when failover is performed from the secondary node to the primary. You have to do this manually with the UPDATE pg_dist_node command. This patch adds the citus_activate_node command to secondary nodes.

@microsoft-github-policy-service agree

[Muhammad Usama (codeforall)]

Thanks for the fix. The bug analysis is correct. I just digged into the original cause of this issue and it seems like a regression from #6728 . Before that refactor, the old ActivateNodeList had explicit NodeIsPrimary guards that skipped metadata sync for secondaries while still setting isactive=true via SetWorkerColumnLocalOnly, and then SetNodeState → SetWorkerColumn propagated the change to other metadata-holding primaries. The refactor lost those guards, so ActivateNodeList now tries to sync metadata/objects to the secondary (a read-only replica), which fails.

The fix direction is right (early return for secondaries), but the new ActivateNode() function reimplements what SetWorkerColumn already does, and IMHO the entire fix can be simplified to something like

if (NodeIsSecondary(workerNode))
{
    EnsureTransactionalMetadataSyncMode();
    SetWorkerColumn(workerNode, Anum_pg_dist_node_isactive, BoolGetDatum(true));
    TransactionModifiedNodeMetadata = true;
    PG_RETURN_INT32(workerNode->nodeId);
}

This would eliminate the new function, and quite a few lines of duplicated code.

[Muhammad Usama (codeforall)]

The new test is added after ensure_no_intermediate_data_leak, which has the comment "This should always be last."
Please move it before that line (e.g., before ensure_no_intermediate_data_leak, after multi_add_node_from_backup_sync_replica).

[Muhammad Usama (codeforall)]

The test adds a secondary node and activates it but doesn't clean up. Consider adding cleanup at the end to avoid polluting state for any future tests that might be added after this one in the schedule:
May be:
-- cleanup
SELECT citus_remove_node('localhost', :follower_worker_2_port);

[Alexandre (akalend)]

Muhammad Usama (@codeforall) , Thanks for review. Your patch is more usable.
All code fixed. Check style is Ok.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.70%. Comparing base (359396c) to head (0a30b98).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8535   +/-   ##
=======================================
  Coverage   88.70%   88.70%           
=======================================
  Files         289      289           
  Lines       64846    64850    +4     
  Branches     8176     8176           
=======================================
+ Hits        57519    57524    +5     
  Misses       4965     4965           
+ Partials     2362     2361    -1     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

Fixes issue #8470 where citus_activate_node() did not reactivate secondary nodes after they were disabled (relevant for failover workflows where a secondary needs to become active again in metadata).

Changes:

• Update citus_activate_node() to explicitly set pg_dist_node.isactive = true when the target node is a secondary.
• Add a new regression test that reproduces the bug and validates activation propagation to metadata nodes.
• Register the new test in the multi-follower regression schedule.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/backend/distributed/metadata/node_metadata.c	Activates secondary nodes by setting isactive and returning early in citus_activate_node().
src/test/regress/sql/check_activate_secondary_node.sql	Adds SQL regression coverage for disabling and re-activating a secondary node (plus error-path checks).
src/test/regress/expected/check_activate_secondary_node.out	Expected output for the new regression test.
src/test/regress/multi_follower_schedule	Adds the new test to the multi-follower schedule.