Use ST_EstimatedExtent for quick bounds calc

[sharkAndshark]

Try to fix #1206

• Use St_EstimatedExtent
• Update test cases

[nyurik]

why in quotes? these values are not strings, they are escaped PostgreSQL identifiers (schema/table/column names)

[sharkAndshark]

Per postgis doc, the arguments type of ST_EstimatedExtent is text.

box2d ST_EstimatedExtent(text schema_name, text table_name, text geocolumn_name);

[nyurik]

interesting, thx, did not realize that. Should the parameters be pre-escaped though? We would need to test this on the MixedCase. Apparently there was a bug 10 years ago -- https://trac.osgeo.org/postgis/ticket/2834 that temporarily used the escaped version, but then it was reverted it seems

[sharkAndshark]

Made a little bit test.

SELECT ST_EstimatedExtent('public','Project_Copy1','geom')
-- BOX(399529.5 3384786.5,412743.5 3394297)

SELECT ST_EstimatedExtent('public','"Project_Copy1"','geom')
-- syntax error

I will add a test on MixedCase later.

[nyurik]

I added a test for weird table names - #1222 -- please review, and maybe add a test for a similar function too, probably in the same namespace.

[nyurik]

Also note this code: https://github.com/postgis/postgis/blob/bccab952f00696d5a48e9eb495e827825eda23a2/postgis/gserialized_estimate.c#L2315 - postgis adds double quotes around passed in schema & table names. I really hope that function returns pre-escaped value without double quotes, or else it will fail spectacularly

[sharkAndshark]

@nyurik any help on this? I think we should call escape_literal before ST_EstimatedExtent, but the tests still fails.
[2024-03-13T04:33:08Z ERROR martin::pg::builder] Failed to create a source: Postgres error while querying table bounds: db error: ERROR: invalid name syntax

[sharkAndshark]

And the SQL is : SELECT ST_EstimatedExtent('"Quotes'' and Space.Dot.', '. Points" ''quote', '. "Geom"') as bounds

[nyurik]

Sigh, one of those painful cases I guess - I created a bug https://trac.osgeo.org/postgis/ticket/5697

So to move forward, I think we will have to do a workaround with the following logic:

• if schema, table, and geom column is "simple" (matches this regex for all 3 values: ^[a-zA-Z_][a-zA-Z_0-9]*$), pass these values "as is", but make sure to pass them as parameters (second param in .query_one())
• if not, use the existing query

[nyurik]

I have managed to get it to work in some cases, but still some issues remain. The trick I think is to remove the " around schema and table, but keep the geometry column as a string value, so something like this, but there is still something wrong

    let params: Vec<&(dyn ToSql + Sync)>;
    let sql: String;

    // Both queries require identifier-escaped format for schema and table
    let schema = escape_identifier(schema);
    let table = escape_identifier(table);
    let mut schema: &str = schema.as_str();
    let mut table: &str = table.as_str();

    if is_quick {
        // Remove first and last quote (but only one on each side)
        schema = &schema[1..schema.len() - 1];
        table = &table[1..table.len() - 1];
        params = vec![&schema, &table, &geometry_column];
        sql = "SELECT ST_EstimatedExtent($1, $2, $3) as bounds".to_string();
    } else {
        params = vec![];
        let geometry_column = escape_identifier(geometry_column);
        sql = format!(
            r#"
WITH real_bounds AS (SELECT ST_SetSRID(ST_Extent({geometry_column}), {srid}) AS rb FROM {schema}.{table})
SELECT ST_Transform(
            CASE
                WHEN (SELECT ST_GeometryType(rb) FROM real_bounds LIMIT 1) = 'ST_Point'
                THEN ST_SetSRID(ST_Extent(ST_Expand({geometry_column}, 1)), {srid})
                ELSE (SELECT * FROM real_bounds)
            END,
            4326
        ) AS bounds
FROM {schema}.{table};
                "#
        );
    };

[nyurik]

I got it to work in https://github.com/sharkAndshark/martin/pull/103/files -- once merged into your branch, it will appear here as well. Let me know what you think

[CommanderStorm]

@nyurik @sharkAndshark
You both worked on this PR and I think you both found the current state good.
Am I misunderstanding something?
Is something still missing?

[CommanderStorm]

(From my POV, this looks good)