On Mon Mar 4, 2024 at 3:05 AM CST, Heikki Linnakangas wrote:
I've now completed many of the side-quests, here are the patches that
remain.
The first three patches form a logical unit. They move the
initialization of the Port struct from postmaster to the backend
process. Currently, that work is split between the postmaster and the
backend process so that postmaster fills in the socket and some other
fields, and the backend process fills the rest after reading the startup
packet. With these patches, there is a new much smaller ClientSocket
struct that is passed from the postmaster to the child process, which
contains just the fields that postmaster initializes. The Port struct is
allocated in the child process. That makes the backend startup easier to
understand. I plan to commit those three patches next if there are no
objections.
That leaves the rest of the patches. I think they're in pretty good
shape too, and I've gotten some review on those earlier and have
addressed the comments I got so far, but would still appreciate another
round of review.
- * *MyProcPort, because ConnCreate() allocated that space with malloc()
- * ... else we'd need to copy the Port data first. Also, subsidiary
data
- * such as the username isn't lost either; see ProcessStartupPacket().
+ * *MyProcPort, because that space is allocated in stack ... else we'd
+ * need to copy the Port data first. Also, subsidiary data such as the
+ * username isn't lost either; see ProcessStartupPacket().
s/allocated in/allocated on the
The first 3 patches seem good to go, in my opinion.
@@ -225,14 +331,13 @@ internal_forkexec(int argc, char *argv[], ClientSocket
*client_sock, BackgroundW
return -1;
}
- /* Make sure caller set up argv properly */
- Assert(argc >= 3);
- Assert(argv[argc] == NULL);
- Assert(strncmp(argv[1], "--fork", 6) == 0);
- Assert(argv[2] == NULL);
-
- /* Insert temp file name after --fork argument */
+ /* set up argv properly */
+ argv[0] = "postgres";
+ snprintf(forkav, MAXPGPATH, "--forkchild=%s", child_kind);
+ argv[1] = forkav;
+ /* Insert temp file name after --forkchild argument */
argv[2] = tmpfilename;
+ argv[3] = NULL;
Should we use postgres_exec_path instead of the naked "postgres" here?
+ /* in postmaster, fork failed ... */
+ ereport(LOG,
+ (errmsg("could not fork worker process: %m")));
+ /* undo what assign_backendlist_entry did */
+ ReleasePostmasterChildSlot(rw->rw_child_slot);
+ rw->rw_child_slot = 0;
+ pfree(rw->rw_backend);
+ rw->rw_backend = NULL;
+ /* mark entry as crashed, so we'll try again later */
+ rw->rw_crashed_at = GetCurrentTimestamp();
+ return false;
I think the error message should include the word "background." It would
be more consistent with the log message above it.
+typedef struct
+{
+ int syslogFile;
+ int csvlogFile;
+ int jsonlogFile;
+} syslogger_startup_data;
It would be nice if all of these startup data structs were named
similarly. For instance, a previous one was BackendStartupInfo. It would
help with greppability.
I noticed there were a few XXX comments left that you created. I'll
highlight them here for more visibility.
+/* XXX: where does this belong? */
+extern bool LoadedSSL;
Perhaps near the My* variables or maybe in the Port struct?
+#ifdef EXEC_BACKEND
+
+ /*
+ * Need to reinitialize the SSL library in the backend, since the
context
+ * structures contain function pointers and cannot be passed through
the
+ * parameter file.
+ *
+ * If for some reason reload fails (maybe the user installed broken key
+ * files), soldier on without SSL; that's better than all connections
+ * becoming impossible.
+ *
+ * XXX should we do this in all child processes? For the moment it's
+ * enough to do it in backend children. XXX good question indeed
+ */
+#ifdef USE_SSL
+ if (EnableSSL)
+ {
+ if (secure_initialize(false) == 0)
+ LoadedSSL = true;
+ else
+ ereport(LOG,
+ (errmsg("SSL configuration could not be
loaded in child process")));
+ }
+#endif
+#endif
Here you added the "good question indeed." I am not sure what the best
answer is either! :)
+ /* XXX: translation? */
+ ereport(LOG,
+ (errmsg("could not fork %s process: %m",
PostmasterChildName(type))));
I assume you are referring to the child name here?
XXX: We now have functions called AuxiliaryProcessInit() and
InitAuxiliaryProcess(). Confusing.
Based on my analysis, the *Init() is called in the Main functions, while
Init*() is called before the Main functions. Maybe
AuxiliaryProcessInit() could be renamed to AuxiliaryProcessStartup()?
Rename the other to AuxiliaryProcessInit().
--
Tristan Partin
Neon (https://neon.tech)
