Re: [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE DATABASE statement

Hello

Is ruleutils.c the best place for this function?

It's already huge, and it has a different scope: "Functions to convert
stored expressions/querytrees back to source text"

+ /* Fetch the value of COLLATION_VERSION */
+ dbvalue = SysCacheGetAttr(DATABASEOID, tuple_database,
+ Anum_pg_database_datcollversion, &attr_isnull);
+ if (!attr_isnull)
+ get_formatted_string(&buf, pretty_flags, 8, "COLLATION_VERSION = %s",
+ quote_literal_cstr(TextDatumGetCString(dbvalue)));

pg_dumpall only shows this for binary upgrade, otherwise skips it. Is
it okay for this command to print it by default, shouldn't it depend
on is_with_defaults or something similar?

+#ifndef DDL_DEFAULTS_H
+#define DDL_DEFAULTS_H
+
+static const struct
+{
+ struct
+ {
....

This file seems strange. A static const struct in a header with
uppercase names doesn't seem to follow postgres conventions?
DATCONNLIMIT_UNLIMITED alredy exists as a definition, and probably
should be used instead or referenced, or the existing uses should
refer to the new way of defining it.

+ /* Fetch the value of LC_COLLATE */
+ dbvalue = SysCacheGetAttr(DATABASEOID, tuple_database,
+ Anum_pg_database_datcollate, &attr_isnull);
+ if (!attr_isnull)
+ get_formatted_string(&buf, pretty_flags, 8, "LC_COLLATE = %s",
+ quote_literal_cstr(TextDatumGetCString(dbvalue)));
+ /* Fetch the value of LC_CTYPE */
+ dbvalue = SysCacheGetAttr(DATABASEOID, tuple_database,
+ Anum_pg_database_datctype, &attr_isnull);

Can these be ever nulls?

Also, pg_dump only emits LOCALE if ctype==collate, shouldn't this
follow the same pattern?

+ else if (is_with_defaults)
+ get_formatted_string(&buf, pretty_flags, 8, "LOCALE_PROVIDER = libc");

Doesn't pg_dump always emit this? Shouldn't this function follow the
same convention? Emitting it seems to be a safer default, in case
postgres ever changes this.

+ /* Build the CREATE DATABASE statement */
+ appendStringInfo(&buf, "CREATE DATABASE %s",
+ quote_identifier(dbform->datname.data));
+ get_formatted_string(&buf, pretty_flags, 4, "WITH");

Shouldn't we only emit "WITH" if it is actually followed by something,
not unconditionally?

+/*
+ * get_formatted_string
+ *
+ * Return a formatted version of the string.

But it's a void function.

+ else if (!attr_isnull)
+ get_formatted_string(&buf, pretty_flags, 8, "LOCALE = %s",
+ quote_literal_cstr(TextDatumGetCString(dbvalue)));
+

Can this ever happen, shouldn't it be an assertion instead?