Thread: initdb / bootstrap design
Hi, [1] reminded me of a topic that I wanted to bring up at some point: To me the division of labor between initdb and bootstrap doesn't make much sense anymore: initdb reads postgres.bki, replaces a few tokens, starts postgres in bootstrap mode, and then painstakenly feeds bootstrap.bki lines to the server. Given that bootstrap mode parsing is a dedicated parser, only invoked from a single point, what's the point of initdb doing the preprocessing and then incurring pipe overhead? Sure, there's a few tokens that we replace in initdb. As it turns out there's only two rows that are actually variable. The username of the initial superuser in pg_authid and the pg_database row for template 1, where encoding, lc_collate and lc_ctype varies. The rest is all compile time constant replacements we could do as part of genbki.pl. It seems we could save a good number of context switches by opening postgres.bki just before boot_yyparse() in BootstrapModeMain() and having the parser read it. The pg_authid / pg_database rows we could just do via explicit insertions in BootstrapModeMain(), provided by commandline args? Similarly, since the introduction of extensions at the latest, the server knows how to execute SQL from a file. Why don't we just process information_schema.sql, system_views.sql et al that way? If we don't need a dedicated "input" mode feeding boot_yyparse() in bootstrap mode anymore (because bootstrap mode feeds it from postgres.bki directly), we likely could avoid the restart between bootstrap and single user mode. Afaics that only really is needed because we need to send SQL after bootstrap_template1(). That'd likely be a nice speedup, because we don't need to write the bootstrap contents from shared buffers to the OS just to read them back in single user mode. I don't plan to work on this immediately, but I thought it's worth bringing up anyway. Greetings, Andres Freund [1] https://www.postgresql.org/message-id/20220216012953.6d7bzmsblqou3ru4%40alap3.anarazel.de
On Wed, Feb 16, 2022 at 9:12 AM Andres Freund <andres@anarazel.de> wrote: > To me the division of labor between initdb and bootstrap doesn't make much > sense anymore: [...] > I don't plan to work on this immediately, but I thought it's worth bringing up > anyway. Added TODO item "Rationalize division of labor between initdb and bootstrap" -- John Naylor EDB: http://www.enterprisedb.com
On 16.02.22 03:12, Andres Freund wrote: > Sure, there's a few tokens that we replace in initdb. As it turns out there's > only two rows that are actually variable. The username of the initial > superuser in pg_authid and the pg_database row for template 1, where encoding, > lc_collate and lc_ctype varies. The rest is all compile time constant > replacements we could do as part of genbki.pl. > > It seems we could save a good number of context switches by opening > postgres.bki just before boot_yyparse() in BootstrapModeMain() and having the > parser read it. The pg_authid / pg_database rows we could just do via > explicit insertions in BootstrapModeMain(), provided by commandline args? I think we could do the locale setup by updating the pg_database row of template1 after bootstrap, as in the attached patch. (The order of proceedings in the surrounding function might need some refinement in a final patch.) I suspect we could do the treatment of pg_authid similarly.
Attachment
Hi, On 2022-02-16 11:47:31 +0100, Peter Eisentraut wrote: > On 16.02.22 03:12, Andres Freund wrote: > > Sure, there's a few tokens that we replace in initdb. As it turns out there's > > only two rows that are actually variable. The username of the initial > > superuser in pg_authid and the pg_database row for template 1, where encoding, > > lc_collate and lc_ctype varies. The rest is all compile time constant > > replacements we could do as part of genbki.pl. > > > > It seems we could save a good number of context switches by opening > > postgres.bki just before boot_yyparse() in BootstrapModeMain() and having the > > parser read it. The pg_authid / pg_database rows we could just do via > > explicit insertions in BootstrapModeMain(), provided by commandline args? > > I think we could do the locale setup by updating the pg_database row of > template1 after bootstrap, as in the attached patch. Another solution could be to have bootstrap create template0 instead of template1. I think for template0 it'd more accurate to have a hardcoded C collation and ascii encoding (which I don't think we actually have?). > I suspect we could do the treatment of pg_authid similarly. Yea. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> Sure, there's a few tokens that we replace in initdb. As it turns out there's
> only two rows that are actually variable. The username of the initial
> superuser in pg_authid and the pg_database row for template 1, where encoding,
> lc_collate and lc_ctype varies. The rest is all compile time constant
> replacements we could do as part of genbki.pl.
I remembered the reason why it's done that way: if we replaced those
values during genbki.pl, the contents of postgres.bki would become
architecture-dependent, belying its distribution as a "share" file.
While we don't absolutely have to continue treating postgres.bki
as architecture-independent, I'm skeptical that there's enough win
here to justify a packaging change.
initdb is already plenty fast enough for any plausible production
usage; it's cases like check-world where we wish it were faster.
So I'm thinking what we really ought to pursue is the idea that's
been kicked around more than once of capturing the post-initdb
state of a cluster's files and just doing "cp -a" to duplicate that
later in the test run.
regards, tom lane
Hi, On 2022-02-16 13:24:41 -0500, Tom Lane wrote: > I remembered the reason why it's done that way: if we replaced those > values during genbki.pl, the contents of postgres.bki would become > architecture-dependent, belying its distribution as a "share" file. > While we don't absolutely have to continue treating postgres.bki > as architecture-independent, I'm skeptical that there's enough win > here to justify a packaging change. Hm. Architecturally I still would like to move it to be processed server side. I'd like to eventually get rid of single user mode (but keep bootstrap, at least for longer). Seems we could make NAMEDATALEN, FLOAT8PASSBYVAL, ALIGNOF_POINTER, FLOAT8PASSBYVAL stuff that bootparse knows about? And remove the need for POSTGRES, ENCODING, LC_COLLATE, LC_CTYPE as discussed already? > initdb is already plenty fast enough for any plausible production > usage; it's cases like check-world where we wish it were faster. It's not just our own usage though. I've seen it be a noticable time in test suites of applications using postgres. And that's not really addressable with the template approach, unless we want to move use of the template database into initdb itself. I've thought about it, but then we'd need to do a lot more than if it's just for our own tests. > So I'm thinking what we really ought to pursue is the idea that's > been kicked around more than once of capturing the post-initdb > state of a cluster's files and just doing "cp -a" to duplicate that > later in the test run. Yea, we should pursue that independently of improving initdb's architecture / speed. initdb will never be as fast as copying files around. I kind of got stuck on how to deal with install.pl / vcregress.pl. For make it's easy enough to create the template during during temp-install. But for the msvc stuff is less clear when / where to create the template database. Nearly everyone uses NO_TEMP_INSTALL on windows, because install is so slow and happens in every test. But right now there's no command to create the "temp" installation. Probably need something like a 'temp-install' command for vcregress.pl and then convert the buildfarm to use that. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2022-02-16 13:24:41 -0500, Tom Lane wrote:
>> I remembered the reason why it's done that way: if we replaced those
>> values during genbki.pl, the contents of postgres.bki would become
>> architecture-dependent, belying its distribution as a "share" file.
> Hm. Architecturally I still would like to move it to be processed server
> side. I'd like to eventually get rid of single user mode (but keep bootstrap,
> at least for longer).
> Seems we could make NAMEDATALEN, FLOAT8PASSBYVAL, ALIGNOF_POINTER,
> FLOAT8PASSBYVAL stuff that bootparse knows about? And remove the need for
> POSTGRES, ENCODING, LC_COLLATE, LC_CTYPE as discussed already?
Yeah, I have no objection to doing it that way. It should be possible
to do those substitutions on a per-field basis, which'd be cleaner than
what initdb does now ...
regards, tom lane
On Wed, Feb 16, 2022 at 2:50 PM Andres Freund <andres@anarazel.de> wrote: > > initdb is already plenty fast enough for any plausible production > > usage; it's cases like check-world where we wish it were faster. > > It's not just our own usage though. I've seen it be a noticable time in test > suites of applications using postgres. I'd just like to second this point. I was working on an EDB proprietary software project for a while which, because of the nature of what it did, ran initdb frequently in its test suite. And it was unbelievably painful. The test suite just took forever. Fortunately, it always ran initdb with the same options, so somebody invented a mechanism for doing one initdb and saving the results someplace and just copying them every time, and it made a huge difference. Before that experience, I probably would have agreed with the idea that there was no need at all for initdb to be any faster than it is already. But, like, what if we'd been trying to run initdb with different options for different tests, the way the core code does? That seems like an entirely plausible thing to want to do, and then caching becomes a real pain. -- Robert Haas EDB: http://www.enterprisedb.com
On 2/17/22 10:36, Robert Haas wrote: > On Wed, Feb 16, 2022 at 2:50 PM Andres Freund <andres@anarazel.de> wrote: >>> initdb is already plenty fast enough for any plausible production >>> usage; it's cases like check-world where we wish it were faster. >> It's not just our own usage though. I've seen it be a noticable time in test >> suites of applications using postgres. > I'd just like to second this point. > > I was working on an EDB proprietary software project for a while > which, because of the nature of what it did, ran initdb frequently in > its test suite. And it was unbelievably painful. The test suite just > took forever. Fortunately, it always ran initdb with the same options, > so somebody invented a mechanism for doing one initdb and saving the > results someplace and just copying them every time, and it made a huge > difference. Before that experience, I probably would have agreed with > the idea that there was no need at all for initdb to be any faster > than it is already. But, like, what if we'd been trying to run initdb > with different options for different tests, the way the core code > does? That seems like an entirely plausible thing to want to do, and > then caching becomes a real pain. Indeed. When initdb.c was written the testing landscape was very different both for the community and for projects that used Postgres. So we need to catch up. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Here's an initial patch that gets rid of the need for initdb to
change the contents of postgres.bki before feeding it to the
bootstrap backend. After this, we could look at having the
backend read the file directly.
I don't really detect any speed change from getting rid of initdb's
string manipulations, but TBH I was not expecting any. On my machine,
that was lost in the noise already, according to perf(1).
regards, tom lane
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 9fa8fdd4cf..667c829064 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -635,6 +635,8 @@ InsertOneTuple(void)
/* ----------------
* InsertOneValue
+ *
+ * Fill the i'th column of the current tuple with the given value.
* ----------------
*/
void
@@ -653,6 +655,21 @@ InsertOneValue(char *value, int i)
elog(DEBUG4, "inserting column %d value \"%s\"", i, value);
+ /*
+ * In order to make the contents of postgres.bki architecture-independent,
+ * certain values in it are represented symbolically, and we perform the
+ * necessary replacements here.
+ */
+ if (strcmp(value, "NAMEDATALEN") == 0)
+ value = CppAsString2(NAMEDATALEN);
+ else if (strcmp(value, "SIZEOF_POINTER") == 0)
+ value = CppAsString2(SIZEOF_VOID_P);
+ else if (strcmp(value, "ALIGNOF_POINTER") == 0)
+ value = (SIZEOF_VOID_P == 4) ? "i" : "d";
+ else if (strcmp(value, "FLOAT8PASSBYVAL") == 0)
+ value = FLOAT8PASSBYVAL ? "true" : "false";
+
+ /* Now convert the value to internal form */
typoid = TupleDescAttr(boot_reldesc->rd_att, i)->atttypid;
boot_get_type_io_data(typoid,
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 97f15971e2..6db9c4f334 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -265,13 +265,13 @@ static void setup_privileges(FILE *cmdfd);
static void set_info_version(void);
static void setup_schema(FILE *cmdfd);
static void load_plpgsql(FILE *cmdfd);
+static void set_remaining_details(FILE *cmdfd);
static void vacuum_db(FILE *cmdfd);
static void make_template0(FILE *cmdfd);
static void make_postgres(FILE *cmdfd);
static void trapsig(int signum);
static void check_ok(void);
static char *escape_quotes(const char *src);
-static char *escape_quotes_bki(const char *src);
static int locale_date_order(const char *locale);
static void check_locale_name(int category, const char *locale,
char **canonname);
@@ -336,32 +336,6 @@ escape_quotes(const char *src)
return result;
}
-/*
- * Escape a field value to be inserted into the BKI data.
- * Run the value through escape_quotes (which will be inverted
- * by the backend's DeescapeQuotedString() function), then wrap
- * the value in single quotes, even if that isn't strictly necessary.
- */
-static char *
-escape_quotes_bki(const char *src)
-{
- char *result;
- char *data = escape_quotes(src);
- char *resultp;
- char *datap;
-
- result = (char *) pg_malloc(strlen(data) + 3);
- resultp = result;
- *resultp++ = '\'';
- for (datap = data; *datap; datap++)
- *resultp++ = *datap;
- *resultp++ = '\'';
- *resultp = '\0';
-
- free(data);
- return result;
-}
-
/*
* make a copy of the array of lines, with token replaced by replacement
* the first time it occurs on each line.
@@ -1357,7 +1331,6 @@ bootstrap_template1(void)
char **line;
char **bki_lines;
char headerline[MAXPGPATH];
- char buf[64];
printf(_("running bootstrap script ... "));
fflush(stdout);
@@ -1379,32 +1352,6 @@ bootstrap_template1(void)
exit(1);
}
- /* Substitute for various symbols used in the BKI file */
-
- sprintf(buf, "%d", NAMEDATALEN);
- bki_lines = replace_token(bki_lines, "NAMEDATALEN", buf);
-
- sprintf(buf, "%d", (int) sizeof(Pointer));
- bki_lines = replace_token(bki_lines, "SIZEOF_POINTER", buf);
-
- bki_lines = replace_token(bki_lines, "ALIGNOF_POINTER",
- (sizeof(Pointer) == 4) ? "i" : "d");
-
- bki_lines = replace_token(bki_lines, "FLOAT8PASSBYVAL",
- FLOAT8PASSBYVAL ? "true" : "false");
-
- bki_lines = replace_token(bki_lines, "POSTGRES",
- escape_quotes_bki(username));
-
- bki_lines = replace_token(bki_lines, "ENCODING",
- encodingid_to_string(encodingid));
-
- bki_lines = replace_token(bki_lines, "LC_COLLATE",
- escape_quotes_bki(lc_collate));
-
- bki_lines = replace_token(bki_lines, "LC_CTYPE",
- escape_quotes_bki(lc_ctype));
-
/* Also ensure backend isn't confused by this environment var: */
unsetenv("PGCLIENTENCODING");
@@ -1622,12 +1569,11 @@ setup_collation(FILE *cmdfd)
static void
setup_privileges(FILE *cmdfd)
{
- char **line;
- char **priv_lines;
- static char *privileges_setup[] = {
+ const char *const *line;
+ static const char *const privileges_setup[] = {
"UPDATE pg_class "
" SET relacl = (SELECT array_agg(a.acl) FROM "
- " (SELECT E'=r/\"$POSTGRES_SUPERUSERNAME\"' as acl "
+ " (SELECT '=r/\"POSTGRES\"' as acl "
" UNION SELECT unnest(pg_catalog.acldefault("
" CASE WHEN relkind = " CppAsString2(RELKIND_SEQUENCE) " THEN 's' "
" ELSE 'r' END::\"char\"," CppAsString2(BOOTSTRAP_SUPERUSERID) "::oid))"
@@ -1759,9 +1705,7 @@ setup_privileges(FILE *cmdfd)
NULL
};
- priv_lines = replace_token(privileges_setup, "$POSTGRES_SUPERUSERNAME",
- escape_quotes(username));
- for (line = priv_lines; *line != NULL; line++)
+ for (line = privileges_setup; *line != NULL; line++)
PG_CMD_PUTS(*line);
}
@@ -1822,6 +1766,48 @@ load_plpgsql(FILE *cmdfd)
PG_CMD_PUTS("CREATE EXTENSION plpgsql;\n\n");
}
+/*
+ * Set some remaining details that aren't known when postgres.bki is made.
+ *
+ * Up to now, the bootstrap superuser has been named "POSTGRES".
+ * Replace that with the user-specified name (often "postgres").
+ * Also, insert the desired locale and encoding details in pg_database.
+ *
+ * Note: this must run after setup_privileges(), which expects the superuser
+ * name to still be "POSTGRES".
+ */
+static void
+set_remaining_details(FILE *cmdfd)
+{
+ char **line;
+ char **detail_lines;
+
+ /*
+ * Ideally we'd change the superuser name with ALTER USER, but the backend
+ * will reject that with "session user cannot be renamed", so we must
+ * cheat. (In any case, we'd need a function to escape an identifier, not
+ * a string literal.) Likewise, we can't change template1's
+ * locale/encoding without cheating.
+ */
+ static char *final_details[] = {
+ "UPDATE pg_authid SET rolname = E'SUPERUSER_NAME' WHERE rolname = 'POSTGRES';\n\n",
+ "UPDATE pg_database SET encoding = E'ENCODING', datcollate = E'LC_COLLATE', datctype = E'LC_CTYPE';\n\n",
+ NULL
+ };
+
+ detail_lines = replace_token(final_details, "SUPERUSER_NAME",
+ escape_quotes(username));
+ detail_lines = replace_token(detail_lines, "ENCODING",
+ encodingid_to_string(encodingid));
+ detail_lines = replace_token(detail_lines, "LC_COLLATE",
+ escape_quotes(lc_collate));
+ detail_lines = replace_token(detail_lines, "LC_CTYPE",
+ escape_quotes(lc_ctype));
+
+ for (line = detail_lines; *line != NULL; line++)
+ PG_CMD_PUTS(*line);
+}
+
/*
* clean everything up in template1
*/
@@ -2851,6 +2837,8 @@ initialize_data_directory(void)
load_plpgsql(cmdfd);
+ set_remaining_details(cmdfd);
+
vacuum_db(cmdfd);
make_template0(cmdfd);
diff --git a/src/include/catalog/pg_database.dat b/src/include/catalog/pg_database.dat
index e7e42d6023..c92cdde260 100644
--- a/src/include/catalog/pg_database.dat
+++ b/src/include/catalog/pg_database.dat
@@ -12,11 +12,14 @@
[
+# We initialize template1's encoding as PG_SQL_ASCII and its locales as C.
+# initdb will change that during database initialization.
+
{ oid => '1', oid_symbol => 'TemplateDbOid',
descr => 'default template for new databases',
- datname => 'template1', encoding => 'ENCODING', datistemplate => 't',
+ datname => 'template1', encoding => '0', datistemplate => 't',
datallowconn => 't', datconnlimit => '-1', datfrozenxid => '0',
- datminmxid => '1', dattablespace => 'pg_default', datcollate => 'LC_COLLATE',
- datctype => 'LC_CTYPE', datacl => '_null_' },
+ datminmxid => '1', dattablespace => 'pg_default', datcollate => 'C',
+ datctype => 'C', datacl => '_null_' },
]
Hi,
On 2022-02-19 18:35:18 -0500, Tom Lane wrote:
> Here's an initial patch that gets rid of the need for initdb to
> change the contents of postgres.bki before feeding it to the
> bootstrap backend. After this, we could look at having the
> backend read the file directly.
Cool!
> I don't really detect any speed change from getting rid of initdb's
> string manipulations, but TBH I was not expecting any. On my machine,
> that was lost in the noise already, according to perf(1).
Yea, I'd not expect much either. The slowdown around the string stuff that I
did see was on windows.
I would however expect some, but not huge, speedup by getting rid of the
line-by-line reading/writing of postgres.bki, even without moving the handling
to the backend.
A quick way to prototype the moving the handlign to the backend would be to
just call postgres with input redirection from postgres.bki...
> + /*
> + * Ideally we'd change the superuser name with ALTER USER, but the backend
> + * will reject that with "session user cannot be renamed", so we must
> + * cheat. (In any case, we'd need a function to escape an identifier, not
> + * a string literal.) Likewise, we can't change template1's
> + * locale/encoding without cheating.
> + */
> + static char *final_details[] = {
> + "UPDATE pg_authid SET rolname = E'SUPERUSER_NAME' WHERE rolname = 'POSTGRES';\n\n",
> + "UPDATE pg_database SET encoding = E'ENCODING', datcollate = E'LC_COLLATE', datctype = E'LC_CTYPE';\n\n",
> + NULL
> + };
> +
> + detail_lines = replace_token(final_details, "SUPERUSER_NAME",
> + escape_quotes(username));
> + detail_lines = replace_token(detail_lines, "ENCODING",
> + encodingid_to_string(encodingid));
> + detail_lines = replace_token(detail_lines, "LC_COLLATE",
> + escape_quotes(lc_collate));
> + detail_lines = replace_token(detail_lines, "LC_CTYPE",
> + escape_quotes(lc_ctype));
Hm, wouldn't it be less code to just use printf?
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
> A quick way to prototype the moving the handlign to the backend would be to
> just call postgres with input redirection from postgres.bki...
Hmm. I was thinking of inventing an include-file command in the
BKI language, and making initdb just send an INCLUDE command.
That's arguably overkill for the immediate need, but it looks like it
requires just a few lines of code (flex provides pretty much all of the
infrastructure already), and maybe we'd find another use for it later.
However, redirection does sound like a very easy answer ...
> Hm, wouldn't it be less code to just use printf?
Meh --- it'd be different from the way we do it in the rest
of initdb, and it would not be "less code". Maybe it'd run
a shade faster, but I refuse to believe that that'd be
enough to matter.
regards, tom lane
Hi, On February 19, 2022 4:39:38 PM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote: >Andres Freund <andres@anarazel.de> writes: >> A quick way to prototype the moving the handlign to the backend would be to >> just call postgres with input redirection from postgres.bki... > >Hmm. I was thinking of inventing an include-file command in the >BKI language, and making initdb just send an INCLUDE command. >That's arguably overkill for the immediate need, but it looks like it >requires just a few lines of code (flex provides pretty much all of the >infrastructure already), and maybe we'd find another use for it later. > >However, redirection does sound like a very easy answer ... Medium term I'd rather do neither, because I'd like to avoid the restart in-between bootstrap and the various sql files.But short term redirection redirection might be good enough - it does mostly work on windows I think ... Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
I wrote:
> However, redirection does sound like a very easy answer ...
I tried it like that (full patch attached) and the results are
intensely disappointing. On my Mac laptop, the time needed for
50 iterations of initdb drops from 16.8 sec to 16.75 sec.
On my RHEL8 workstation, the change is actually in the wrong
direction, from 18.75s to 18.9s. I conclude that the time
spent on postgres.bki data transfer is so far down in the noise
as to be overwhelmed by irrelevancies. (Which, in fact, is
what perf told me before I started --- but I'd hoped that the
number of system calls would diminish noticeably. Seems not.)
Not sure that this is worth pursuing any further.
regards, tom lane
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 9fa8fdd4cf..667c829064 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -635,6 +635,8 @@ InsertOneTuple(void)
/* ----------------
* InsertOneValue
+ *
+ * Fill the i'th column of the current tuple with the given value.
* ----------------
*/
void
@@ -653,6 +655,21 @@ InsertOneValue(char *value, int i)
elog(DEBUG4, "inserting column %d value \"%s\"", i, value);
+ /*
+ * In order to make the contents of postgres.bki architecture-independent,
+ * certain values in it are represented symbolically, and we perform the
+ * necessary replacements here.
+ */
+ if (strcmp(value, "NAMEDATALEN") == 0)
+ value = CppAsString2(NAMEDATALEN);
+ else if (strcmp(value, "SIZEOF_POINTER") == 0)
+ value = CppAsString2(SIZEOF_VOID_P);
+ else if (strcmp(value, "ALIGNOF_POINTER") == 0)
+ value = (SIZEOF_VOID_P == 4) ? "i" : "d";
+ else if (strcmp(value, "FLOAT8PASSBYVAL") == 0)
+ value = FLOAT8PASSBYVAL ? "true" : "false";
+
+ /* Now convert the value to internal form */
typoid = TupleDescAttr(boot_reldesc->rd_att, i)->atttypid;
boot_get_type_io_data(typoid,
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 97f15971e2..9850f342bf 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -265,13 +265,13 @@ static void setup_privileges(FILE *cmdfd);
static void set_info_version(void);
static void setup_schema(FILE *cmdfd);
static void load_plpgsql(FILE *cmdfd);
+static void set_remaining_details(FILE *cmdfd);
static void vacuum_db(FILE *cmdfd);
static void make_template0(FILE *cmdfd);
static void make_postgres(FILE *cmdfd);
static void trapsig(int signum);
static void check_ok(void);
static char *escape_quotes(const char *src);
-static char *escape_quotes_bki(const char *src);
static int locale_date_order(const char *locale);
static void check_locale_name(int category, const char *locale,
char **canonname);
@@ -336,32 +336,6 @@ escape_quotes(const char *src)
return result;
}
-/*
- * Escape a field value to be inserted into the BKI data.
- * Run the value through escape_quotes (which will be inverted
- * by the backend's DeescapeQuotedString() function), then wrap
- * the value in single quotes, even if that isn't strictly necessary.
- */
-static char *
-escape_quotes_bki(const char *src)
-{
- char *result;
- char *data = escape_quotes(src);
- char *resultp;
- char *datap;
-
- result = (char *) pg_malloc(strlen(data) + 3);
- resultp = result;
- *resultp++ = '\'';
- for (datap = data; *datap; datap++)
- *resultp++ = *datap;
- *resultp++ = '\'';
- *resultp = '\0';
-
- free(data);
- return result;
-}
-
/*
* make a copy of the array of lines, with token replaced by replacement
* the first time it occurs on each line.
@@ -1354,22 +1328,26 @@ static void
bootstrap_template1(void)
{
PG_CMD_DECL;
- char **line;
- char **bki_lines;
+ FILE *infile;
char headerline[MAXPGPATH];
- char buf[64];
+ char firstline[MAXPGPATH];
printf(_("running bootstrap script ... "));
fflush(stdout);
- bki_lines = readfile(bki_file);
-
/* Check that bki file appears to be of the right version */
snprintf(headerline, sizeof(headerline), "# PostgreSQL %s\n",
PG_MAJORVERSION);
- if (strcmp(headerline, *bki_lines) != 0)
+ if ((infile = fopen(bki_file, "r")) == NULL)
+ {
+ pg_log_error("could not open file \"%s\" for reading: %m", bki_file);
+ exit(1);
+ }
+
+ if (fgets(firstline, sizeof(firstline), infile) == NULL ||
+ strcmp(headerline, firstline) != 0)
{
pg_log_error("input file \"%s\" does not belong to PostgreSQL %s",
bki_file, PG_VERSION);
@@ -1379,56 +1357,24 @@ bootstrap_template1(void)
exit(1);
}
- /* Substitute for various symbols used in the BKI file */
-
- sprintf(buf, "%d", NAMEDATALEN);
- bki_lines = replace_token(bki_lines, "NAMEDATALEN", buf);
-
- sprintf(buf, "%d", (int) sizeof(Pointer));
- bki_lines = replace_token(bki_lines, "SIZEOF_POINTER", buf);
-
- bki_lines = replace_token(bki_lines, "ALIGNOF_POINTER",
- (sizeof(Pointer) == 4) ? "i" : "d");
-
- bki_lines = replace_token(bki_lines, "FLOAT8PASSBYVAL",
- FLOAT8PASSBYVAL ? "true" : "false");
-
- bki_lines = replace_token(bki_lines, "POSTGRES",
- escape_quotes_bki(username));
-
- bki_lines = replace_token(bki_lines, "ENCODING",
- encodingid_to_string(encodingid));
-
- bki_lines = replace_token(bki_lines, "LC_COLLATE",
- escape_quotes_bki(lc_collate));
-
- bki_lines = replace_token(bki_lines, "LC_CTYPE",
- escape_quotes_bki(lc_ctype));
+ fclose(infile);
/* Also ensure backend isn't confused by this environment var: */
unsetenv("PGCLIENTENCODING");
snprintf(cmd, sizeof(cmd),
- "\"%s\" --boot -X %d %s %s %s %s",
+ "\"%s\" --boot -X %d %s %s %s %s <\"%s\"",
backend_exec,
wal_segment_size_mb * (1024 * 1024),
data_checksums ? "-k" : "",
boot_options, extra_options,
- debug ? "-d 5" : "");
-
+ debug ? "-d 5" : "",
+ bki_file);
PG_CMD_OPEN;
-
- for (line = bki_lines; *line != NULL; line++)
- {
- PG_CMD_PUTS(*line);
- free(*line);
- }
-
+ /* Nothing to write, since backend reads bki_file directly */
PG_CMD_CLOSE;
- free(bki_lines);
-
check_ok();
}
@@ -1622,12 +1568,11 @@ setup_collation(FILE *cmdfd)
static void
setup_privileges(FILE *cmdfd)
{
- char **line;
- char **priv_lines;
- static char *privileges_setup[] = {
+ const char *const *line;
+ static const char *const privileges_setup[] = {
"UPDATE pg_class "
" SET relacl = (SELECT array_agg(a.acl) FROM "
- " (SELECT E'=r/\"$POSTGRES_SUPERUSERNAME\"' as acl "
+ " (SELECT '=r/\"POSTGRES\"' as acl "
" UNION SELECT unnest(pg_catalog.acldefault("
" CASE WHEN relkind = " CppAsString2(RELKIND_SEQUENCE) " THEN 's' "
" ELSE 'r' END::\"char\"," CppAsString2(BOOTSTRAP_SUPERUSERID) "::oid))"
@@ -1759,9 +1704,7 @@ setup_privileges(FILE *cmdfd)
NULL
};
- priv_lines = replace_token(privileges_setup, "$POSTGRES_SUPERUSERNAME",
- escape_quotes(username));
- for (line = priv_lines; *line != NULL; line++)
+ for (line = privileges_setup; *line != NULL; line++)
PG_CMD_PUTS(*line);
}
@@ -1822,6 +1765,48 @@ load_plpgsql(FILE *cmdfd)
PG_CMD_PUTS("CREATE EXTENSION plpgsql;\n\n");
}
+/*
+ * Set some remaining details that aren't known when postgres.bki is made.
+ *
+ * Up to now, the bootstrap superuser has been named "POSTGRES".
+ * Replace that with the user-specified name (often "postgres").
+ * Also, insert the desired locale and encoding details in pg_database.
+ *
+ * Note: this must run after setup_privileges(), which expects the superuser
+ * name to still be "POSTGRES".
+ */
+static void
+set_remaining_details(FILE *cmdfd)
+{
+ char **line;
+ char **detail_lines;
+
+ /*
+ * Ideally we'd change the superuser name with ALTER USER, but the backend
+ * will reject that with "session user cannot be renamed", so we must
+ * cheat. (In any case, we'd need a function to escape an identifier, not
+ * a string literal.) Likewise, we can't change template1's
+ * locale/encoding without cheating.
+ */
+ static char *final_details[] = {
+ "UPDATE pg_authid SET rolname = E'SUPERUSER_NAME' WHERE rolname = 'POSTGRES';\n\n",
+ "UPDATE pg_database SET encoding = E'ENCODING', datcollate = E'LC_COLLATE', datctype = E'LC_CTYPE';\n\n",
+ NULL
+ };
+
+ detail_lines = replace_token(final_details, "SUPERUSER_NAME",
+ escape_quotes(username));
+ detail_lines = replace_token(detail_lines, "ENCODING",
+ encodingid_to_string(encodingid));
+ detail_lines = replace_token(detail_lines, "LC_COLLATE",
+ escape_quotes(lc_collate));
+ detail_lines = replace_token(detail_lines, "LC_CTYPE",
+ escape_quotes(lc_ctype));
+
+ for (line = detail_lines; *line != NULL; line++)
+ PG_CMD_PUTS(*line);
+}
+
/*
* clean everything up in template1
*/
@@ -2851,6 +2836,8 @@ initialize_data_directory(void)
load_plpgsql(cmdfd);
+ set_remaining_details(cmdfd);
+
vacuum_db(cmdfd);
make_template0(cmdfd);
diff --git a/src/include/catalog/pg_database.dat b/src/include/catalog/pg_database.dat
index e7e42d6023..87aad30146 100644
--- a/src/include/catalog/pg_database.dat
+++ b/src/include/catalog/pg_database.dat
@@ -12,11 +12,15 @@
[
+# We initialize template1's encoding as PG_SQL_ASCII and its locales as C.
+# initdb will change that during database initialization; however, the
+# post-bootstrap initialization session will run with those values.
+
{ oid => '1', oid_symbol => 'TemplateDbOid',
descr => 'default template for new databases',
- datname => 'template1', encoding => 'ENCODING', datistemplate => 't',
+ datname => 'template1', encoding => 'PG_SQL_ASCII', datistemplate => 't',
datallowconn => 't', datconnlimit => '-1', datfrozenxid => '0',
- datminmxid => '1', dattablespace => 'pg_default', datcollate => 'LC_COLLATE',
- datctype => 'LC_CTYPE', datacl => '_null_' },
+ datminmxid => '1', dattablespace => 'pg_default', datcollate => 'C',
+ datctype => 'C', datacl => '_null_' },
]
diff --git a/src/include/catalog/pg_database.h b/src/include/catalog/pg_database.h
index 76adbd4aad..9fd424d58d 100644
--- a/src/include/catalog/pg_database.h
+++ b/src/include/catalog/pg_database.h
@@ -38,7 +38,7 @@ CATALOG(pg_database,1262,DatabaseRelationId) BKI_SHARED_RELATION BKI_ROWTYPE_OID
Oid datdba BKI_DEFAULT(POSTGRES) BKI_LOOKUP(pg_authid);
/* character encoding */
- int32 encoding;
+ int32 encoding BKI_LOOKUP(encoding);
/* allowed as CREATE DATABASE template? */
bool datistemplate;
Hi, On 2022-02-19 20:46:26 -0500, Tom Lane wrote: > I tried it like that (full patch attached) and the results are intensely > disappointing. On my Mac laptop, the time needed for 50 iterations of > initdb drops from 16.8 sec to 16.75 sec. Hm. I'd hoped for at least a little bit bigger win. But I think it enables more, see below: > Not sure that this is worth pursuing any further. I experimented with moving all the bootstrapping into --boot mode and got it working. Albeit definitely with a few hacks (more below). While I had hoped for a bit more of a win, it's IMO a nice improvement. Executing 10 initdb -N --wal-segsize 1 in a loop: HEAD: assert: 8.06user 1.17system 0:09.25elapsed 99%CPU (0avgtext+0avgdata 91724maxresident)k 0inputs+549280outputs (40major+99824minor)pagefaults 0swaps opt: 2.89user 0.99system 0:04.81elapsed 80%CPU (0avgtext+0avgdata 88864maxresident)k 0inputs+549280outputs (40major+99792minor)pagefaults 0swaps default to lz4: assert: 7.61user 1.03system 0:08.69elapsed 99%CPU (0avgtext+0avgdata 91508maxresident)k 0inputs+546400outputs (42major+99551minor)pagefaults 0swaps opt: 2.55user 0.94system 0:03.49elapsed 99%CPU (0avgtext+0avgdata 88816maxresident)k 0inputs+546400outputs (40major+99551minor)pagefaults 0swaps bootstrap replace: assert: 7.42user 1.00system 0:08.52elapsed 98%CPU (0avgtext+0avgdata 91656maxresident)k 0inputs+546400outputs (40major+97737minor)pagefaults 0swaps opt: 2.49user 0.98system 0:03.49elapsed 99%CPU (0avgtext+0avgdata 88700maxresident)k 0inputs+546400outputs (40major+97728minor)pagefaults 0swaps everything in bootstrap: assert: 6.31user 0.94system 0:07.35elapsed 98%CPU (0avgtext+0avgdata 97812maxresident)k 0inputs+547360outputs (30major+88617minor)pagefaults 0swaps opt: 2.42user 0.85system 0:03.28elapsed 99%CPU (0avgtext+0avgdata 94572maxresident)k 0inputs+547360outputs (30major+83712minor)pagefaults 0swaps optimize WAL in bootstrap: assert: 6.26user 0.96system 0:07.29elapsed 99%CPU (0avgtext+0avgdata 97844maxresident)k 0inputs+547360outputs (30major+88586minor)pagefaults 0swaps opt: 2.43user 0.80system 0:03.24elapsed 99%CPU (0avgtext+0avgdata 94436maxresident)k 0inputs+547360outputs (30major+83664minor)pagefaults 0swaps remote isatty in bootstrap: assert: 6.15user 0.83system 0:06.99elapsed 99%CPU (0avgtext+0avgdata 97832maxresident)k 0inputs+465120outputs (30major+88559minor)pagefaults 0swaps opt: 2.28user 0.85system 0:03.14elapsed 99%CPU (0avgtext+0avgdata 94604maxresident)k 0inputs+465120outputs (30major+83728minor)pagefaults 0swaps That's IMO not bad. On windows I see a higher gains, which makes sense, because filesystem IO is slower. Freebsd as well, but the variance is oddly high, so I might be doing something wrong. The main reason I like this however isn't the speedup itself, but that after this initdb doesn't depend on single user mode at all anymore. About the prototype: - Most of the bootstrap SQL is executed from bootstrap.c itself. But some still comes from the client. E.g. password, a few information_schema details and the database / authid changes. - To execute the sql I mostly used extension.c's read_whole_file()/execute_sql_string(). But VACUUM, CREATE DATABASE require all the transactional hacks in portal.c etc. So I wrapped exec_simple_query() for that phase. Might be better to just call vacuum.c / database.c directly. - for indexed relcache access to work the phase of RelationCacheInitializePhase3() that's initially skipped needs to be executed. I hacked that up by adding a RelationCacheInitializePhase3b() that bootstrap.c can call, but that's obviously too ugly to live. - InvalidateSystemCaches() is needed after bki processing. Otherwise I see an "row is too big:" error. Didn't investigate yet. - I definitely removed some validation that we'd probably want. But that seems something to care about later... - 0004 prevents a fair bit of WAL from being written. While XLogInsert did some of that, it didn't block FPIs, which obviously are bulky. This reduces WAL from ~5MB to ~100kB. There's quite a bit of further speedup potential: - One bottleneck, particularly in optimized mode, is the handling of huge node trees for views. strToNode() and nodeRead() are > 10% alone - Enabling index access sometime during the postgres.bki processing would make invalidation handling for subsequent indexes faster. Or maybe we can disable a few more invalidations. Inval processing is >10% - more than 10% (assert) / 7% (optimized) is spent in compute_scalar_stats()->qsort_arg(). Something seems off with that to me. Completely crazy? Greetings, Andres Freund
Attachment
- v1-0001-Set-default_toast_compression-lz4-if-available.patch
- v1-0002-initdb-move-token-replacing-in-postgres.bki-to-ba.patch
- v1-0003-initdb-perform-everything-during-boot-mostly-in-b.patch
- v1-0004-initdb-Optimize-WAL-writing-during-initdb.patch
- v1-0005-initdb-call-isatty-only-once-in-bootparse.y.patch
On 20.02.22 01:39, Tom Lane wrote: >> Hm, wouldn't it be less code to just use printf? > Meh --- it'd be different from the way we do it in the rest > of initdb, and it would not be "less code". Maybe it'd run > a shade faster, but I refuse to believe that that'd be > enough to matter. There is a PG_CMD_PRINTF() that is used for that purpose.