Set default schema to 'public' and use qualified names everywhere

joto · joto · commit e59440cad34a · 2023-08-07T15:56:43.000+02:00
Until now the default schema was empty and the PostgreSQL search path was used to find tables etc. We now set the default schema for the middle and the pgsql and flex outputs to 'public'. This simplifies the code, because we don't have to handle the case without schema specially any more. More importantly it makes it easier for the user to reason about what osm2pgsql is doing, because it does not depend on the setting of the search path in the PostgreSQL session. This fixes a problem where osm2pgsql could not find the osm2pgsql_properties table it created itself, because it was created in a different schema than expected (#2010). The behaviour of the gazetteer output has not changed. It still uses unqualified names. This will be a breaking change for those users who rely on the old behaviour. Fixes #2010, #2011.
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
