Start Maintenance Daemon for Main DB at the server start. #7254

emelsimsek · 2023-10-12T11:19:44Z

DESCRIPTION: This change starts a maintenance deamon at the time of server start if there is a designated main database.

This is the code flow:

User designated an admin database:
ALTER SYSTEM SET citus.main_db = "myadmindb";
When postmaster starts, in _PG_Init, citus calls
InitializeMaintenanceDaemonForMainDb

This function registers a background worker to run CitusMaintenanceDaemonMain with databaseOid = 0
CitusMaintenanceDaemonMain takes some special actions when databaseOid is 0:
- Gets the citus.main_db value.
- Connects to the citus.main_db
- Now the MyDatabaseId is available, creates a hash entry for it.
- Then follows the same control flow as for a regular db,

src/backend/distributed/utils/maintenanced.c

src/backend/distributed/shared_library_init.c

JelteF · 2023-10-13T10:21:09Z

src/backend/distributed/utils/maintenanced.c

@@ -146,6 +147,12 @@ InitializeMaintenanceDaemon(void)
 void
 InitializeMaintenanceDaemonForAdminDB(void)
 {
+	if (strcmp(ControlDbName, "") == 0)


Are GUCs already read in _PG_init? If not, this would always return true.

Yes, it appears that they are read since this change is able to start a maintenance deamon in the controldb even if there is no connection.

It is also possible to move this check to CitusMaintenanceDaemonMain, Then a background worker will start but then quit immediately.

codecov · 2023-10-18T10:00:17Z

Codecov Report

Merging #7254 (daa4cee) into main (10198b1) will decrease coverage by 0.11%.
The diff coverage is 93.33%.

❗ Current head daa4cee differs from pull request most recent head 1e805da. Consider uploading reports for the commit 1e805da to get more accurate results

@@            Coverage Diff             @@
##             main    #7254      +/-   ##
==========================================
- Coverage   93.22%   93.11%   -0.11%     
==========================================
  Files         275      275              
  Lines       59491    61397    +1906     
  Branches        0     7421    +7421     
==========================================
+ Hits        55459    57169    +1710     
- Misses       4032     4163     +131     
- Partials        0       65      +65

halilozanakgul · 2023-10-19T12:24:33Z

src/backend/distributed/shared_library_init.c

@@ -481,6 +481,7 @@ _PG_init(void)
 #endif

 	InitializeMaintenanceDaemon();
+	InitializeMaintenanceDaemonForMainDb();


Do you treat DB as a word? Should it be ...ForMainDb() or...ForMainDB()?

Both styles seem to be used in the codebase. E.g:

bcf54c5#:~:text=MaintenanceDaemonDBData%20*)-,MaintenanceDaemonDBData%20*myDbData%20%3D%20(MaintenanceDaemonDBData%20*),HASH_FIND%2C%20NULL)%3B,-if%20(!myDbData

I do not have a preference.

JelteF · 2023-10-23T14:55:13Z

src/test/regress/citus_tests/test/test_maintenancedeamon.py

+def test_set_maindb(coord):
+    with coord.cur() as cur1:
+        cur1.execute("DROP DATABASE IF EXISTS mymaindb;")
+        cur1.execute("CREATE DATABASE mymaindb;")


Let's track the databases we create so we can clean them up automatically, just like we do for schemas. See create_schema, cleanup_schemas and cleanup_test_leftovers in src/test/regress/citus_tests/common.py

That should also make the DROP DATABASE IF EXISTS unnecessary.

JelteF · 2023-10-23T15:05:59Z

src/backend/distributed/utils/maintenanced.c

+	SafeSnprintf(worker.bgw_name, sizeof(worker.bgw_name),
+				 "Citus Maintenance Daemon: %u/%u",
+				 0, 0);


I think instead of 0/0 it's more useful to users to write out what it is for.

Suggested change

SafeSnprintf(worker.bgw_name, sizeof(worker.bgw_name),

"Citus Maintenance Daemon: %u/%u",

0, 0);

strcpy_s(worker.bgw_name, sizeof(worker.bgw_name),

"Citus Maintenance Daemon for Main DB");

JelteF · 2023-10-23T15:47:43Z

src/backend/distributed/utils/maintenanced.c

+			/*
+			 * When the database crashes, background workers are restarted, but
+			 * the state in shared memory is lost. In that case, we exit and wait
+			 * for Postmaster calling __PG_Init which in turn calls
+			 * InitializeMaintenanceDaemonForAdminDB.
+			 */


This comment is actually not true here. Here' we're checking for insert failure, i.e. MaintenaceDaemonHash being full. We should throw a useful error here about that, because that's quite unexpected.

I actually think it might be good to move this insertion logic (including zero-ing) to a function that is called here and by InitializeMaintenanceDaemonBackend.

I think my main source of finding this code confusing is the two branches (if (databaseOid == 0) and else) that seem similar, but are actually quite different. I think it would help readability to split these a bit better. Either by putting their bodies in a function, or creating a new CitusMaintenanceDaemonMain function for the databaseOid == 0 case. These could then call some functions for the shared code. But I think having these long branches but quite different branches make the code hard to follow, since now there's two paths that you need to look at carefully.

JelteF · 2023-10-25T09:26:35Z

src/test/regress/citus_tests/test/test_maintenancedeamon.py

+        == 0
+    )
+
+    cluster.coordinator.cleanup_databases()


cleanup_databases should be called in the cleanup_test_leftovers function so that it is called even when the test fails.

JelteF · 2023-10-27T09:12:46Z

src/backend/distributed/utils/maintenanced.c

+ * ConnectToDatabase connects to the database for the given databaseOid.
+ * if databaseOid is 0, connects to MainDb and then creates a hash entry.
+ * If a hash entry cannot be created for MainDb it exists the process requesting a restart.
+ * However for regular databases, it exits without requesting a restart since another
+ * subsequent backend should be starting the Maintenance Deamon.
+ * If a hash entry found has a valid workerPid, it exits
+ * without requesting a restart since there is already a deamon running.


rewording + typo fixes

Suggested change

* ConnectToDatabase connects to the database for the given databaseOid.

* if databaseOid is 0, connects to MainDb and then creates a hash entry.

* If a hash entry cannot be created for MainDb it exists the process requesting a restart.

* However for regular databases, it exits without requesting a restart since another

* subsequent backend should be starting the Maintenance Deamon.

* If a hash entry found has a valid workerPid, it exits

* without requesting a restart since there is already a deamon running.

* ConnectToDatabase connects to the database for the given databaseOid.

* if databaseOid is 0, connects to MainDb and then creates a hash entry.

* If a hash entry cannot be created for MainDb it exists the process requesting a restart.

* However for regular databases, it exits without requesting a restart since another

* subsequent backend should be starting the Maintenance Daemon.

* If the found hash entry has a valid workerPid, it exits

* without requesting a restart since there is already a daemon running.

JelteF

Left a small suggestions about the wording in the codecomment. But other than that this looks good now.

DESCRIPTION: This change starts a maintenance deamon at the time of server start if there is a designated main database. This is the code flow: 1. User designates a main database: `ALTER SYSTEM SET citus.main_db = "myadmindb";` 2. When postmaster starts, in _PG_Init, citus calls `InitializeMaintenanceDaemonForMainDb` This function registers a background worker to run `CitusMaintenanceDaemonMain `with `databaseOid = 0 ` 3. `CitusMaintenanceDaemonMain ` takes some special actions when databaseOid is 0: - Gets the citus.main_db value. - Connects to the citus.main_db - Now the `MyDatabaseId `is available, creates a hash entry for it. - Then follows the same control flow as for a regular db,

emelsimsek requested a review from JelteF October 12, 2023 11:21

onurctirtir mentioned this pull request Oct 13, 2023

Adds 2PC distributed commands from other databases #7203

Merged

JelteF reviewed Oct 13, 2023

View reviewed changes

src/backend/distributed/utils/maintenanced.c Show resolved Hide resolved

emelsimsek changed the title ~~Start Maintenance Daemon in Control DB at the server start.~~ Start Maintenance Daemon for Control DB at the server start. Oct 13, 2023

JelteF reviewed Oct 13, 2023

View reviewed changes

src/backend/distributed/shared_library_init.c Outdated Show resolved Hide resolved

JelteF reviewed Oct 13, 2023

View reviewed changes

emelsimsek requested a review from JelteF October 17, 2023 07:43

halilozanakgul reviewed Oct 19, 2023

View reviewed changes

emelsimsek changed the title ~~Start Maintenance Daemon for Control DB at the server start.~~ Start Maintenance Daemon for Main DB at the server start. Oct 20, 2023

JelteF reviewed Oct 23, 2023

View reviewed changes

emelsimsek marked this pull request as ready for review October 23, 2023 18:01

emelsimsek force-pushed the StartMaintenanceDaemonInControlDB branch from b28a6c8 to 1e805da Compare October 23, 2023 18:36

JelteF reviewed Oct 25, 2023

View reviewed changes

emelsimsek requested a review from JelteF October 27, 2023 07:39

JelteF reviewed Oct 27, 2023

View reviewed changes

JelteF approved these changes Oct 27, 2023

View reviewed changes

ivyazmitinov mentioned this pull request Oct 27, 2023

7244/multi db connection improvements #7286

Open

emelsimsek added 10 commits October 30, 2023 09:29

Start Daemon in server startup

87a5fcf

Fix some bugs

0ba4729

Add controldb_name GUC

67d6246

controldb_name should require retsart

c26415c

Refactor error handling

b14c5a0

Fix comments and error handling

995cb06

Rename GUC

d076b43

Fix deadlock in regress

3d88054

Reindent

8c033ec

Style

6d7648c

emelsimsek added 19 commits October 30, 2023 09:29

reindent

72899c5

Add python test

c28dae5

Add python test

ab6c68e

Fix typo

9b295b9

Fix flakyness

075de6a

Fix flakyness

ff817d8

Fix flakyness

ba0377c

test cleanup

d6e75f6

Refactor

0a0382b

Fix process name

afa24ed

Refactor

731e9c8

Change tests

bbf316b

Reformat

ea4e9c3

Wait until deamon starts in tests

4836ede

Improve the test

11a13cc

Add cleanup_databases to cleanup_test_leftovers

9f6e0b8

Refactor

297ef20

Comments

0607039

Fix typos

2565296

emelsimsek force-pushed the StartMaintenanceDaemonInControlDB branch from a210359 to 2565296 Compare October 30, 2023 06:29

emelsimsek merged commit ee8f4bb into main Oct 30, 2023
153 of 154 checks passed

emelsimsek deleted the StartMaintenanceDaemonInControlDB branch October 30, 2023 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start Maintenance Daemon for Main DB at the server start. #7254

Start Maintenance Daemon for Main DB at the server start. #7254

emelsimsek commented Oct 12, 2023 •

edited

Loading

JelteF Oct 13, 2023

emelsimsek Oct 16, 2023

codecov bot commented Oct 18, 2023 •

edited

Loading

halilozanakgul Oct 19, 2023

emelsimsek Oct 20, 2023

JelteF Oct 23, 2023 •

edited

Loading

JelteF Oct 23, 2023

JelteF Oct 23, 2023 •

edited

Loading

JelteF Oct 23, 2023 •

edited

Loading

JelteF Oct 23, 2023

JelteF Oct 25, 2023

JelteF Oct 27, 2023

JelteF left a comment

Start Maintenance Daemon for Main DB at the server start. #7254

Start Maintenance Daemon for Main DB at the server start. #7254

Conversation

emelsimsek commented Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 18, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelteF Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelteF Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

JelteF Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelteF left a comment

Choose a reason for hiding this comment

emelsimsek commented Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 18, 2023 •

edited

Loading

JelteF Oct 23, 2023 •

edited

Loading

JelteF Oct 23, 2023 •

edited

Loading

JelteF Oct 23, 2023 •

edited

Loading