Merge pull request #2027 from joto/schema-qualification

lonvia · web-flow · commit d887a2ea7d9a · 2023-08-10T10:17:21.000+02:00
Set default schema to 'public' and use qualified names everywhere
diff --git a/docs/osm2pgsql.md b/docs/osm2pgsql.md
@@ -165,7 +165,7 @@ mandatory for short options too.
 
 \--middle-schema=SCHEMA
 :   Use PostgreSQL schema SCHEMA for all tables, indexes, and functions in
-    the middle (default is no schema, i.e. the `public` schema is used).
+    the middle (default is `public`).
 
 \--middle-way-node-index-id-shift=SHIFT
 :   Set ID shift for way node bucket index in middle. Experts only. See
@@ -275,8 +275,7 @@ mandatory for short options too.
 
 \--output-pgsql-schema=SCHEMA
 :   Use PostgreSQL schema SCHEMA for all tables, indexes, and functions in
-    the pgsql output (default is no schema, i.e. the `public` schema
-    is used).
+    the pgsql output (default is `public`).
 
 # EXPIRE OPTIONS
 
diff --git a/src/command-line-parser.cpp b/src/command-line-parser.cpp
@@ -181,7 +181,7 @@ Middle options:\n\
        --cache-strategy=STRATEGY  Deprecated. Not used any more.\n\
     -x|--extra-attributes  Include attributes (user name, user id, changeset\n\
                     id, timestamp and version) for each object in the database.\n\
-       --middle-schema=SCHEMA  Schema to use for middle tables (default: none).\n\
+       --middle-schema=SCHEMA  Schema to use for middle tables (default: 'public').\n\
        --middle-way-node-index-id-shift=SHIFT  Set ID shift for bucket index.\n\
        --middle-database-format=FORMAT  Set middle db format (default: legacy).\n\
        --middle-with-nodes  Store tagged nodes in db (new middle db format only).\n\
@@ -213,7 +213,7 @@ Pgsql output options:\n\
     -K|--keep-coastlines  Keep coastline data rather than filtering it out.\n\
                     Default: discard objects tagged natural=coastline.\n\
        --output-pgsql-schema=SCHEMA Schema to use for pgsql output tables\n\
-                    (default: none).\n\
+                    (default: 'public').\n\
        --reproject-area  Compute area column using web mercator coordinates.\n\
 \n\
 Expiry options:\n\
@@ -716,11 +716,17 @@ options_t parse_command_line(int argc, char *argv[])
             return options;
         case 215: // --middle-schema
             options.middle_dbschema = optarg;
+            if (options.middle_dbschema.empty()) {
+                throw std::runtime_error{"Schema can not be empty."};
+            }
             check_identifier(options.middle_dbschema,
                              "--middle-schema parameter");
             break;
         case 216: // --output-pgsql-schema
             options.output_dbschema = optarg;
+            if (options.output_dbschema.empty()) {
+                throw std::runtime_error{"Schema can not be empty."};
+            }
             check_identifier(options.output_dbschema,
                              "--output-pgsql-schema parameter");
             break;
diff --git a/src/db-copy.hpp b/src/db-copy.hpp
@@ -28,8 +28,8 @@
  */
 struct db_target_descr_t
 {
-    /// Schema of the target table (can be empty for default schema)
-    std::string schema;
+    /// Schema of the target table.
+    std::string schema{"public"};
     /// Name of the target table for the copy operation.
     std::string name;
     /// Name of id column used when deleting objects.
diff --git a/src/expire-output.hpp b/src/expire-output.hpp
@@ -12,6 +12,7 @@
 
 #include "tile.hpp"
 
+#include <cassert>
 #include <cstdint>
 #include <string>
 #include <utility>
@@ -39,6 +40,7 @@ class expire_output_t
 
     void set_schema_and_table(std::string schema, std::string table)
     {
+        assert(!schema.empty());
         m_schema = std::move(schema);
         m_table = std::move(table);
     }
@@ -78,8 +80,8 @@ class expire_output_t
     /// The filename (if any) for output
     std::string m_filename;
 
-    /// The schema (if any) for output
-    std::string m_schema;
+    /// The schema for output
+    std::string m_schema{"public"};
 
     /// The table (if any) for output
     std::string m_table;
diff --git a/src/flex-lua-expire-output.cpp b/src/flex-lua-expire-output.cpp
@@ -30,8 +30,8 @@ create_expire_output(lua_State *lua_state,
     lua_pop(lua_state, 1); // "filename"
 
     // optional "schema" and "table" fields
-    auto const *schema =
-        luaX_get_table_string(lua_state, "schema", -1, "The expire output", "");
+    auto const *schema = luaX_get_table_string(lua_state, "schema", -1,
+                                               "The expire output", "public");
     check_identifier(schema, "schema field");
     auto const *table =
         luaX_get_table_string(lua_state, "table", -2, "The expire output", "");
diff --git a/src/gen/gen-base.cpp b/src/gen/gen-base.cpp
@@ -20,10 +20,8 @@ gen_base_t::gen_base_t(pg_conn_t *connection, params_t *params)
     assert(connection);
     assert(params);
 
+    params->check_identifier_with_default("schema", "public");
     auto const schema = params->get_identifier("schema");
-    if (schema.empty()) {
-        params->set("schema", "public");
-    }
 
     if (params->has("src_table")) {
         auto const src_table = get_params().get_identifier("src_table");
diff --git a/src/gen/gen-tile-builtup.cpp b/src/gen/gen-tile-builtup.cpp
@@ -33,7 +33,6 @@ gen_tile_builtup_t::gen_tile_builtup_t(pg_conn_t *connection, params_t *params)
 {
     check_src_dest_table_params_exist();
 
-    m_schema = get_params().get_identifier("schema");
     m_source_tables =
         osmium::split_string(get_params().get_string("src_tables"), ',');
 
@@ -108,10 +107,11 @@ CREATE TABLE IF NOT EXISTS "{}" (
             m_image_buffer, m_margin);
 
     int n = 0;
+    auto const schema = get_params().get_string("schema");
     for (auto const &src_table : m_source_tables) {
         params_t tmp_params;
         tmp_params.set("N", std::to_string(n++));
-        tmp_params.set("SRC", qualified_name(m_schema, src_table));
+        tmp_params.set("SRC", qualified_name(schema, src_table));
 
         dbexec(tmp_params, R"(
 PREPARE get_geoms_{N} (real, real, real, real) AS
diff --git a/src/gen/gen-tile-builtup.hpp b/src/gen/gen-tile-builtup.hpp
@@ -37,8 +37,6 @@ class gen_tile_builtup_t final : public gen_tile_t
 
     std::vector<std::string> m_source_tables;
     std::string m_image_path;
-    std::string m_schema;
-    std::string m_dest_table;
     std::string m_image_table;
     double m_margin = 0.0;
     std::size_t m_image_extent = 2048;
diff --git a/src/gen/osm2pgsql-gen.cpp b/src/gen/osm2pgsql-gen.cpp
@@ -90,6 +90,7 @@ Main Options:
     -c|--create           Run in create mode (default)
     -S|--style=FILE       The Lua config file (required, same as for osm2pgsql)
     -j|--jobs=NUM         Number of parallel jobs (default 1)
+       --middle-schema=SCHEMA  Database schema for middle tables
 
 Help/Version Options:
     -h|--help             Print this help text and stop
@@ -125,6 +126,7 @@ static std::array<option, 20> const long_options = {
      {"log-level", required_argument, nullptr, 'l'},
      {"style", required_argument, nullptr, 'S'},
      {"log-sql", no_argument, nullptr, 201},
+     {"middle-schema", required_argument, nullptr, 202},
      {nullptr, 0, nullptr, 0}}};
 
 struct tile_extent
@@ -583,6 +585,7 @@ int main(int argc, char *argv[])
 {
     try {
         database_options_t database_options;
+        std::string schema{"public"};
         std::string log_level;
         std::string style;
         uint32_t jobs = 1;
@@ -594,7 +597,7 @@ int main(int argc, char *argv[])
         while (-1 != (c = getopt_long(argc, argv, short_options,
                                       long_options.data(), nullptr))) {
             switch (c) {
-            case 'h':
+            case 'h': // --help
                 show_help();
                 return 0;
             case 'a': // --append
@@ -622,19 +625,27 @@ int main(int argc, char *argv[])
             case 'P': // --port
                 database_options.port = optarg;
                 break;
-            case 'l':
+            case 'l': // --log-level
                 log_level = optarg;
                 break;
-            case 'S':
+            case 'S': // --style
                 style = optarg;
                 break;
-            case 'V':
+            case 'V': // --version
                 log_info("osm2pgsql-gen version {}", get_osm2pgsql_version());
                 canvas_t::info();
                 return 0;
-            case 201:
+            case 201: // --log-sql
                 get_logger().enable_sql();
                 break;
+            case 202: // --middle-schema
+                schema = optarg;
+                if (schema.empty()) {
+                    log_error("Schema must not be empty");
+                    return 2;
+                }
+                check_identifier(schema, "--middle-schema");
+                break;
             default:
                 log_error("Unknown argument");
                 return 2;
@@ -696,7 +707,7 @@ int main(int argc, char *argv[])
             init_database_capabilities(db_connection);
         }
 
-        properties_t properties{conninfo, ""};
+        properties_t properties{conninfo, schema};
         properties.load();
 
         bool const updatable = properties.get_bool("updatable", false);
diff --git a/src/middle-pgsql.cpp b/src/middle-pgsql.cpp
@@ -87,9 +87,7 @@ static std::string build_sql(options_t const &options, std::string const &templ)
                                            : "USING INDEX TABLESPACE " +
                                                  options.tblsslim_index};
 
-    std::string const schema = options.middle_dbschema.empty()
-                                   ? ""
-                                   : ("\"" + options.middle_dbschema + "\".");
+    std::string const schema = "\"" + options.middle_dbschema + "\".";
 
     return fmt::format(
         fmt::runtime(templ), fmt::arg("prefix", options.prefix),
diff --git a/src/options.hpp b/src/options.hpp
@@ -98,11 +98,11 @@ struct options_t
     /// Pg Tablespace to store slim tables (no default TABLESPACE)
     std::string tblsslim_data{};
 
-    /// Pg schema to store middle tables in, default none
-    std::string middle_dbschema{};
+    /// Pg schema to store middle tables in.
+    std::string middle_dbschema{"public"};
 
-    /// Pg schema to store output tables in, default none
-    std::string output_dbschema{};
+    /// Pg schema to store output tables in.
+    std::string output_dbschema{"public"};
 
     std::string style{DEFAULT_STYLE}; ///< style file to use
 
diff --git a/src/osm2pgsql.cpp b/src/osm2pgsql.cpp
@@ -264,9 +264,7 @@ static void check_for_nodes_table(options_t const &options)
         return;
     }
 
-    if (!has_table(options.middle_dbschema.empty() ? "public"
-                                                   : options.middle_dbschema,
-                   options.prefix + "_nodes")) {
+    if (!has_table(options.middle_dbschema, options.prefix + "_nodes")) {
         throw std::runtime_error{"You seem to not have a nodes table. Did "
                                  "you forget the --flat-nodes option?"};
     }
diff --git a/src/pgsql-capabilities.cpp b/src/pgsql-capabilities.cpp
@@ -160,9 +160,6 @@ bool has_index_method(std::string const &value)
 
 bool has_table(std::string schema, std::string const &name)
 {
-    if (schema.empty()) {
-        schema = "public";
-    }
     schema += '.';
     schema += name;
 
diff --git a/src/pgsql.cpp b/src/pgsql.cpp
@@ -205,20 +205,8 @@ std::string tablespace_clause(std::string const &name)
 
 std::string qualified_name(std::string const &schema, std::string const &name)
 {
-    std::string result{"\""};
-
-    if (!schema.empty()) {
-        result.reserve(schema.size() + name.size() + 5);
-        result += schema;
-        result += "\".\"";
-    } else {
-        result.reserve(name.size() + 2);
-    }
-
-    result += name;
-    result += '"';
-
-    return result;
+    assert(!schema.empty());
+    return fmt::format(R"("{}"."{}")", schema, name);
 }
 
 void check_identifier(std::string const &name, char const *in)
diff --git a/src/properties.cpp b/src/properties.cpp
@@ -9,19 +9,21 @@
 
 #include "properties.hpp"
 
+#include "format.hpp"
 #include "logging.hpp"
 #include "pgsql-capabilities.hpp"
 #include "pgsql.hpp"
 
+#include <cassert>
 #include <cstdlib>
 
 static constexpr char const *const properties_table = "osm2pgsql_properties";
 
 properties_t::properties_t(std::string conninfo, std::string schema)
 : m_conninfo(std::move(conninfo)), m_schema(std::move(schema)),
-  m_has_properties_table(
-      has_table(m_schema.empty() ? "public" : m_schema, properties_table))
+  m_has_properties_table(has_table(m_schema, properties_table))
 {
+    assert(!m_schema.empty());
     log_debug("Found properties table '{}': {}.", properties_table,
               m_has_properties_table);
 }
@@ -161,15 +163,5 @@ bool properties_t::load()
 
 std::string properties_t::table_name() const
 {
-    std::string table;
-
-    if (!m_schema.empty()) {
-        table += '"';
-        table += m_schema;
-        table += '"';
-        table += '.';
-    }
-    table += properties_table;
-
-    return table;
+    return qualified_name(m_schema, properties_table);
 }
diff --git a/src/table.cpp b/src/table.cpp
@@ -8,6 +8,7 @@
  */
 
 #include <algorithm>
+#include <cassert>
 #include <cstdint>
 #include <cstdio>
 #include <limits>
@@ -34,6 +35,7 @@ table_t::table_t(std::string const &name, std::string type, columns_t columns,
   m_hstore_mode(hstore_mode), m_columns(std::move(columns)),
   m_hstore_columns(std::move(hstore_columns)), m_copy(copy_thread)
 {
+    assert(!schema.empty());
     m_target->schema = schema;
 
     // if we dont have any columns
diff --git a/tests/bdd/flex/lua-expire-output-definitions.feature b/tests/bdd/flex/lua-expire-output-definitions.feature
@@ -40,7 +40,7 @@ Feature: Expire output definitions in Lua file
         Then running osm2pgsql flex fails
         And the error output contains
             """
-            The expire output field must contain a 'schema' string field (or nil for default: '').
+            The expire output field must contain a 'schema' string field (or nil for default: 'public').
             """
 
     Scenario: Table in expire output definition has to be a string
diff --git a/tests/test-middle.cpp b/tests/test-middle.cpp
@@ -128,7 +128,7 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default,
                        "pronamespace = (SELECT oid FROM "
                        "pg_catalog.pg_namespace WHERE nspname = 'public')");
 
-    if (!options.middle_dbschema.empty()) {
+    if (options.middle_dbschema == "osm") {
         conn.exec("CREATE SCHEMA IF NOT EXISTS osm;");
     }
 
@@ -329,7 +329,7 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default,
         REQUIRE_FALSE(mid_q->relation_get(999, &outbuf));
     }
 
-    if (!options.middle_dbschema.empty()) {
+    if (options.middle_dbschema != "public") {
         REQUIRE(num_tables == conn.get_count("pg_catalog.pg_tables",
                                              "schemaname = 'public'"));
         REQUIRE(num_indexes == conn.get_count("pg_catalog.pg_indexes",
diff --git a/tests/test-pgsql-capabilities.cpp b/tests/test-pgsql-capabilities.cpp
@@ -48,10 +48,8 @@ TEST_CASE("has_index_method() should work")
 TEST_CASE("has_table() should work")
 {
     init_database_capabilities(db.db().connect());
-    REQUIRE_FALSE(has_table("", "foo"));
     REQUIRE_FALSE(has_table("public", "foo"));
     REQUIRE_FALSE(has_table("someschema", "foo"));
-    REQUIRE(has_table("", "spatial_ref_sys"));
     REQUIRE(has_table("public", "spatial_ref_sys"));
 }
 
diff --git a/tests/test-pgsql.cpp b/tests/test-pgsql.cpp
@@ -24,9 +24,9 @@ TEST_CASE("Tablespace clause with tablespace")
     REQUIRE(tablespace_clause("foo") == R"( TABLESPACE "foo")");
 }
 
-TEST_CASE("Table name without schema")
+TEST_CASE("Table name with public schema")
 {
-    REQUIRE(qualified_name("", "foo") == R"("foo")");
+    REQUIRE(qualified_name("public", "foo") == R"("public"."foo")");
 }
 
 TEST_CASE("Table name with schema")
diff --git a/tests/test-properties.cpp b/tests/test-properties.cpp
