Update openstreetmap-carto-hstore-only-l10n.lua - Switching from add_row() to insert() for osm2pgsql

[astridx]

Background:

I came across the open question in Issue #4977 regarding the potential impact on the German style of OpenStreetMap. To investigate further, I set up a test environment and tested the installation of the openstreetmap.de tile server using the latest version of osm2pgsql.

$ osm2pgsql --version
osm2pgsql version 1.11.0 (1.11.0-136-gf8c78ae3)
Build: RelWithDebInfo
Compiled using the following library versions:
Libosmium 2.20.0
Proj 9.1.1
Lua 5.3.6

During the data import process using the following command:

sudo -u _tirex osm2pgsql -c -d osm --slim -C 0 -O flex \
  --number-processes 1 -S /srv/tile/sources/osml10n/openstreetmap-carto-hstore-only-l10n.lua \
  /srv/tile/planet.osm.pbf

I encountered the following error:

ERROR: Error loading lua config: ...sources/osml10n/openstreetmap-carto-hstore-only-l10n.lua:81: Error in 'define_table': Unknown column type 'area'. (https://github.com/giggls/osml10n/blob/master/openstreetmap-carto-hstore-only-l10n.lua#L81)

Solution:

Sarah pointed me to this tutorial, which I have now integrated into the Lua script.

Notes:

This version is likely not perfect; there is redundant code, specifically in the functions add_multiroads and add_roads (add_polygons and add_polygons), which could potentially be combined into a single function by checking the type. However, the current implementation works because the "multi"-function is always called from process_relation, ensuring that the types align correctly and my knowledge of the types is limited, and since this solution works, I'm submitting it for your review as is.

[joto]

As you mention there is still quite some code duplication here. Two (mutually exclusive) ideas for you to consider:

1. All the add_* functions have a tags and a new object. If you keep the object parameter, then the tags parameter is superfluous, because you can just call object.tags to get it.
2. Consider using a geom parameter instead of the object parameter. This is initialized outside the function with object:as_linestring() or object:as_multipolygon() or whatever. Then there are fewer versions of those functions, because inside you can use that geometry and do the merging and segmentizing and so on. So you don't need two sets of functions for the non-multi and the multi case.

Also the basic settings of the cols should really go into its own function, it seems to be the same everywhere. But that was already a problem in the old code.

[astridx]

As you mention there is still quite some code duplication here. Two (mutually exclusive) ideas for you to consider:

1. All the `add_*` functions have a `tags` and a new `object`. If you keep the object parameter, then the tags parameter is superfluous, because you can just call `object.tags` to get it.

2. Consider using a `geom` parameter instead of the `object` parameter. This is initialized outside the function with `object:as_linestring()` or `object:as_multipolygon()` or whatever. Then there are fewer versions of those functions, because inside you can use that geometry and do the merging and segmentizing and so on. So you don't need two sets of functions for the non-multi and the multi case.

Also the basic settings of the cols should really go into its own function, it seems to be the same everywhere. But that was already a problem in the old code.

Thanks for the tip @joto . I have tackled the second variant here:
a897f7f

Why do you mean/suggest that it is better to place the basic settings of the column in a separate function?

[joto]

It looks to me that this here is the same in all functions, it it not?

    local cols = {}
    cols.tags = tags
    cols['layer'] = layer(tags['layer'])
    cols['z_order'] = z_order(tags)
    cols.name_l10n = name_l10n

So it makes sense to factor this out and do it in one function for all cases.

[giggls]

Just for your info. This code is derived from an earlier version of a legacy compatible flex output port which @pnorman did for upstream osm-carto adapted to the hstore-only setup I use in German style.

[joto]

@giggls I hope you know that I did a newer version of that. See the issue and this PR. There is a chance that this will get merged at some point. Ideally we would get this in sync with what you are using in some way, for instance by creating specific hook points for extensions.

[giggls]

I do. However, the database layout of German style predates the upstream usage of hstore. It differs in the way that I moved everything to hstore while upstream kept the columns it had before just adding hstore for the rest. I liked my version more and while I was working with views anyway I just kept what I had. I am still able to render upstream though.