On 4/21/24 00:19, Tom Lane wrote:
> Tomas Vondra <tomas.von...@enterprisedb.com> writes:
>> On 4/20/24 22:40, Tom Lane wrote:
>>> Seems reasonable. The alternative could be to remove createdb.c's use
>>> of fmtId() here, but I don't think that's actually better.
>
>> Why? It seems to me this is quite close to e.g. LOCALE_PROVIDER, and we
>> don't do fmtId() for that. So why should we do that for STRATEGY?
>
> Hah, nothing like being randomly inconsistent with adjacent code.
> Every other argument handled by createdb gets wrapped by either
> fmtId or appendStringLiteralConn.
>
> I think the argument for this is it ensures that the switch value as
> accepted by createdb is the argument that CREATE DATABASE will see.
> Compare
>
> $ createdb --strategy="foo bar" mydb
> createdb: error: database creation failed: ERROR: invalid create database
> strategy "foo bar"
> HINT: Valid strategies are "wal_log", and "file_copy".
>
> $ createdb --locale-provider="foo bar" mydb
> createdb: error: database creation failed: ERROR: syntax error at or near ";"
> LINE 1: CREATE DATABASE mydb LOCALE_PROVIDER foo bar;
> ^
>
> I'm not suggesting that this is an interesting security vulnerability,
> because if you can control the arguments to createdb it's probably
> game over long since. But wrapping the arguments is good for
> delivering on-point error messages. So I'd add a fmtId() call to
> LOCALE_PROVIDER too.
>
> BTW, someone's taken the Oxford comma too far in that HINT.
> Nobody writes a comma when there are only two alternatives.
>
OK, the attached 0001 patch does these three things - adds the fmtId()
for locale_provider, make the comparison case-insensitive for strategy
and also removes the comma from the hint.
The createdb vs. CREATE DATABASE difference made me look if we have any
regression tests for CREATE DATABASE, and we don't. I guess it would be
good to have some, so I added a couple, for some of the parameters, see
0002. But there's a problem with the locale stuff - this seems to work
in plain "make check", but pg_upgrade creates the clusters with
different providers etc. which changes the expected output. I'm not sure
there's a good way to deal with this ...
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From e7a18c780591673592b2a5eb1e129fcceb17f0fe Mon Sep 17 00:00:00 2001
From: Tomas Vondra <to...@2ndquadrant.com>
Date: Sun, 21 Apr 2024 14:19:32 +0200
Subject: [PATCH 1/2] createdb: compare strategy case-insensitive
When specifying the createdb strategy, the documentation suggests valid
options are FILE_COPY and WAL_LOG, but the code does case-sensitive
comparison and accepts only "file_copy" and "wal_log" as valid.
Fixed by doing a case-insensitive comparison using pg_strcasecmp(), same
as for other string parameters.
While at it, apply fmtId() to a nearby "locale_provider". This already
did the comparison in case-insensitive way, but the value would not be
double-quoted, confusing the parser and the error message.
Backpatch to 15, where the strategy was introduced.
Backpatch-through: 15
---
src/backend/commands/dbcommands.c | 6 +++---
src/bin/scripts/createdb.c | 2 +-
src/bin/scripts/t/020_createdb.pl | 10 ++++++++++
3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 8229dfa1f22..cd06d1270c5 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -1018,15 +1018,15 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
char *strategy;
strategy = defGetString(dstrategy);
- if (strcmp(strategy, "wal_log") == 0)
+ if (pg_strcasecmp(strategy, "wal_log") == 0)
dbstrategy = CREATEDB_WAL_LOG;
- else if (strcmp(strategy, "file_copy") == 0)
+ else if (pg_strcasecmp(strategy, "file_copy") == 0)
dbstrategy = CREATEDB_FILE_COPY;
else
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid create database strategy \"%s\"", strategy),
- errhint("Valid strategies are \"wal_log\", and \"file_copy\".")));
+ errhint("Valid strategies are \"wal_log\" and \"file_copy\".")));
}
/* If encoding or locales are defaulted, use source's setting */
diff --git a/src/bin/scripts/createdb.c b/src/bin/scripts/createdb.c
index 007061e756f..30426860efa 100644
--- a/src/bin/scripts/createdb.c
+++ b/src/bin/scripts/createdb.c
@@ -237,7 +237,7 @@ main(int argc, char *argv[])
appendStringLiteralConn(&sql, lc_ctype, conn);
}
if (locale_provider)
- appendPQExpBuffer(&sql, " LOCALE_PROVIDER %s", locale_provider);
+ appendPQExpBuffer(&sql, " LOCALE_PROVIDER %s", fmtId(locale_provider));
if (icu_locale)
{
appendPQExpBufferStr(&sql, " ICU_LOCALE ");
diff --git a/src/bin/scripts/t/020_createdb.pl b/src/bin/scripts/t/020_createdb.pl
index 512b55c48a9..4a0e2c883a1 100644
--- a/src/bin/scripts/t/020_createdb.pl
+++ b/src/bin/scripts/t/020_createdb.pl
@@ -255,11 +255,21 @@ $node->issues_sql_like(
qr/statement: CREATE DATABASE foobar6 STRATEGY wal_log TEMPLATE foobar2/,
'create database with WAL_LOG strategy');
+$node->issues_sql_like(
+ [ 'createdb', '-T', 'foobar2', '-S', 'WAL_LOG', 'foobar6s' ],
+ qr/statement: CREATE DATABASE foobar6s STRATEGY "WAL_LOG" TEMPLATE foobar2/,
+ 'create database with WAL_LOG strategy');
+
$node->issues_sql_like(
[ 'createdb', '-T', 'foobar2', '-S', 'file_copy', 'foobar7' ],
qr/statement: CREATE DATABASE foobar7 STRATEGY file_copy TEMPLATE foobar2/,
'create database with FILE_COPY strategy');
+$node->issues_sql_like(
+ [ 'createdb', '-T', 'foobar2', '-S', 'FILE_COPY', 'foobar7s' ],
+ qr/statement: CREATE DATABASE foobar7s STRATEGY "FILE_COPY" TEMPLATE foobar2/,
+ 'create database with FILE_COPY strategy');
+
# Create database owned by role_foobar.
$node->issues_sql_like(
[ 'createdb', '-T', 'foobar2', '-O', 'role_foobar', 'foobar8' ],
--
2.44.0
From 1f7e1925410f1f74e599fda43a8c775401182863 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <to...@2ndquadrant.com>
Date: Sun, 21 Apr 2024 14:28:46 +0200
Subject: [PATCH 2/2] add regression tests for CREATE DATABASE
---
src/test/regress/expected/create_database.out | 55 +++++++++++++++++++
src/test/regress/parallel_schedule | 2 +-
src/test/regress/sql/create_database.sql | 44 +++++++++++++++
3 files changed, 100 insertions(+), 1 deletion(-)
create mode 100644 src/test/regress/expected/create_database.out
create mode 100644 src/test/regress/sql/create_database.sql
diff --git a/src/test/regress/expected/create_database.out b/src/test/regress/expected/create_database.out
new file mode 100644
index 00000000000..241f178a370
--- /dev/null
+++ b/src/test/regress/expected/create_database.out
@@ -0,0 +1,55 @@
+-- error cases
+CREATE DATABASE regress_create_db_1 TEMPLATE regress_db_notexisting;
+ERROR: template database "regress_db_notexisting" does not exist
+CREATE DATABASE regress_create_db_1 ENCODING 'incorrect-encoding';
+ERROR: incorrect-encoding is not a valid encoding name
+LINE 1: CREATE DATABASE regress_create_db_1 ENCODING 'incorrect-enco...
+ ^
+CREATE DATABASE regress_create_db_1 STRATEGY incorrect_strategy;
+ERROR: invalid create database strategy "incorrect_strategy"
+HINT: Valid strategies are "wal_log" and "file_copy".
+CREATE DATABASE regress_create_db_1 LOCALE 'incorrect-locale';
+ERROR: invalid LC_COLLATE locale name: "incorrect-locale"
+HINT: If the locale name is specific to ICU, use ICU_LOCALE.
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER 'incorrect-locale-provider';
+ERROR: unrecognized locale provider: incorrect-locale-provider
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER builtin;
+ERROR: LOCALE or BUILTIN_LOCALE must be specified
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER builtin TEMPLATE template0;
+ERROR: LOCALE or BUILTIN_LOCALE must be specified
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER builtin LOCALE 'incorrect-locale';
+ERROR: invalid LC_COLLATE locale name: "incorrect-locale"
+HINT: If the locale name is specific to ICU, use ICU_LOCALE.
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER builtin BUILTIN_LOCALE 'incorrect-locale';
+ERROR: invalid locale name "incorrect-locale" for builtin provider
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER builtin BUILTIN_LOCALE "C";
+ERROR: new locale provider (builtin) does not match locale provider of the template database (libc)
+HINT: Use the same locale provider as in the template database, or use template0 as template.
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER libc BUILTIN_LOCALE "C";
+ERROR: BUILTIN_LOCALE cannot be specified unless locale provider is builtin
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER libc LOCALE "C";
+ERROR: new collation (C) is incompatible with the collation of the template database (C.UTF-8)
+HINT: Use the same collation as in the template database, or use template0 as template.
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER libc LOCALE "C" ENCODING "UTF-8";
+ERROR: new collation (C) is incompatible with the collation of the template database (C.UTF-8)
+HINT: Use the same collation as in the template database, or use template0 as template.
+CREATE DATABASE regress_create_db_1;
+DROP DATABASE regress_create_db_1;
+CREATE DATABASE regress_create_db_1 ENCODING 'UTF-8';
+DROP DATABASE regress_create_db_1;
+CREATE DATABASE regress_create_db_1 STRATEGY wal_log;
+DROP DATABASE regress_create_db_1;
+CREATE DATABASE regress_create_db_1 STRATEGY WAL_LOG;
+DROP DATABASE regress_create_db_1;
+CREATE DATABASE regress_create_db_1 STRATEGY file_copy;
+DROP DATABASE regress_create_db_1;
+CREATE DATABASE regress_create_db_1 STRATEGY FILE_COPY;
+DROP DATABASE regress_create_db_1;
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER libc;
+DROP DATABASE regress_create_db_1;
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER libc LOCALE "C.UTF-8";
+DROP DATABASE regress_create_db_1;
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER builtin BUILTIN_LOCALE "C" TEMPLATE template0;
+DROP DATABASE regress_create_db_1;
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER libc;
+DROP DATABASE regress_create_db_1;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 675c5676171..f4e99a1775e 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -78,7 +78,7 @@ test: brin_bloom brin_multi
# psql depends on create_am
# amutils depends on geometry, create_index_spgist, hash_index, brin
# ----------
-test: create_table_like alter_generic alter_operator misc async dbsize merge misc_functions sysviews tsrf tid tidscan tidrangescan collate.utf8 collate.icu.utf8 incremental_sort create_role without_overlaps
+test: create_table_like alter_generic alter_operator misc async dbsize merge misc_functions sysviews tsrf tid tidscan tidrangescan collate.utf8 collate.icu.utf8 incremental_sort create_database create_role without_overlaps
# collate.linux.utf8 and collate.icu.utf8 tests cannot be run in parallel with each other
test: rules psql psql_crosstab amutils stats_ext collate.linux.utf8 collate.windows.win1252
diff --git a/src/test/regress/sql/create_database.sql b/src/test/regress/sql/create_database.sql
new file mode 100644
index 00000000000..d4b7b194551
--- /dev/null
+++ b/src/test/regress/sql/create_database.sql
@@ -0,0 +1,44 @@
+-- error cases
+CREATE DATABASE regress_create_db_1 TEMPLATE regress_db_notexisting;
+CREATE DATABASE regress_create_db_1 ENCODING 'incorrect-encoding';
+CREATE DATABASE regress_create_db_1 STRATEGY incorrect_strategy;
+CREATE DATABASE regress_create_db_1 LOCALE 'incorrect-locale';
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER 'incorrect-locale-provider';
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER builtin;
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER builtin TEMPLATE template0;
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER builtin LOCALE 'incorrect-locale';
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER builtin BUILTIN_LOCALE 'incorrect-locale';
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER builtin BUILTIN_LOCALE "C";
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER libc BUILTIN_LOCALE "C";
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER libc LOCALE "C";
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER libc LOCALE "C" ENCODING "UTF-8";
+
+CREATE DATABASE regress_create_db_1;
+DROP DATABASE regress_create_db_1;
+
+CREATE DATABASE regress_create_db_1 ENCODING 'UTF-8';
+DROP DATABASE regress_create_db_1;
+
+CREATE DATABASE regress_create_db_1 STRATEGY wal_log;
+DROP DATABASE regress_create_db_1;
+
+CREATE DATABASE regress_create_db_1 STRATEGY WAL_LOG;
+DROP DATABASE regress_create_db_1;
+
+CREATE DATABASE regress_create_db_1 STRATEGY file_copy;
+DROP DATABASE regress_create_db_1;
+
+CREATE DATABASE regress_create_db_1 STRATEGY FILE_COPY;
+DROP DATABASE regress_create_db_1;
+
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER libc;
+DROP DATABASE regress_create_db_1;
+
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER libc LOCALE "C.UTF-8";
+DROP DATABASE regress_create_db_1;
+
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER builtin BUILTIN_LOCALE "C" TEMPLATE template0;
+DROP DATABASE regress_create_db_1;
+
+CREATE DATABASE regress_create_db_1 LOCALE_PROVIDER libc;
+DROP DATABASE regress_create_db_1;
--
2.44.0
