re-work get_multi_columns

[gordthompson]

Fixes: #297
Fixes: #303

[gordthompson]

ping @rafiss

[rafiss]

Thanks for working on this! I had a few questions/comments about this, some due to my lack of familiarity.

Also some potential additional test cases that might be useful to add:

• A few type mapping tests: BYTEA→BLOB, NUMERIC→DECIMAL precision/scale propagation, REAL→FLOAT, SMALLINT→INTEGER, and all 8 ARRAY variants (those mutate fields in place, so worth having some specific testing for that).
• Non-default schema path for get_multi_columns(schema=...).
• Computed column reflection (Computed("a + b")). The old code emitted computed={sqltext, persisted}; so we could verify that happens still

[gordthompson]

Switching this to draft pending resolution of cockroachdb/cockroach#170049

cc: @rafiss

[gordthompson]

re:

A few type mapping tests: BYTEA→BLOB, NUMERIC→DECIMAL precision/scale propagation, REAL→FLOAT, SMALLINT→INTEGER, and all 8 ARRAY variants (those mutate fields in place, so worth having some specific testing for that).

We have tests like that in test/test_introspection.py . In fact, they are what tipped me off to the need tor our own type mapping in the first place.

Computed column reflection (Computed("a + b")). The old code emitted computed={sqltext, persisted}; so we could verify that happens still

Done.

[NotJustPizza]

@gordthompson @rafiss Any chance to get this merged anytime soon?

[gordthompson]

@gordthompson @rafiss Any chance to get this merged anytime soon?

This change depends in part on the fix for cockroachdb/cockroach#170049 which is included in CRDB v26.3 (verified with v26.3.0-alpha.1), so final merge will likely wait until v26.3.0 is officially released.

In the meantime, we can use

pip install git+https://github.com/gordthompson/sqlalchemy-cockroachdb@issue_297

to get the updated version of this dialect.

[gordthompson]

ping @rafiss

[rafiss]

thanks! my round of comments this time should hopefully not require large changes to address.

[rafiss]

thanks for the replies! my only major concern now is about running one query per table. my other comments are optional suggestions.

[rafiss]

I saw the latest commit. That moves the construction of the select object out of the loop; but the connection.execute() is still inside it, so we're still doing one round trip per table.

On a cluster with tens of thousands of tables (a real use case some of our important users have, not just hypothetical), a MetaData.reflect() would issue tens of thousands of extra round trips on top of upstream's single bulk query. I'm worried that running a query per table would effectively never finish.

I don't think we have to choose between search_path correctness and one query, though. The upstream code uses pg_table_is_visible to resolve schema=None. it's the same predicate in _pg_class_filter_scope_schema that produced the very rows we're post-processing:

https://github.com/sqlalchemy/sqlalchemy/blob/rel_2_0_54/lib/sqlalchemy/dialects/postgresql/base.py#L3653-L3658

So CRDB already evaluates that once per reflection today, and I think we can use it the same way instead of resolving each name with its own ::regclass round trip:

visible_names = [t[1] for t, _ in multi_columns
                 if t[0] is None and t not in self.multi_entries_to_ignore]
qry = (
    select(
        pg_class.c.relname,
        info_schema_columns.c.column_name,
        info_schema_columns.c.is_hidden,
    )
    .select_from(
        pg_class.join(
            pg_namespace, pg_class.c.relnamespace == pg_namespace.c.oid
        ).join(
            info_schema_columns,
            and_(
                pg_namespace.c.nspname == info_schema_columns.c.table_schema,
                pg_class.c.relname == info_schema_columns.c.table_name,
            ),
        )
    )
    .where(pg_class.c.relname.in_(visible_names))
    .where(pg_catalog.pg_table_is_visible(pg_class.c.oid))
    .where(pg_namespace.c.nspname != "pg_catalog")
    .where(pg_namespace.c.nspname != "crdb_internal")
    .where(pg_namespace.c.nspname != "information_schema")
)

keyed on (row.relname, row.column_name). That key is unambiguous without knowing the schema: only one relation of a given name is visible at a time.

Tables that arrived with an explicit schema can keep the tuple_(table_schema, table_name).in_(...) form from the previous revision, which was already correct for that case. Two bulk queries total, regardless of table count.

One bonus: this retires the hand-rolled escaping too, since relname.in_(visible_names) sends the names as bind parameters. The current escaping code does look correct (the two .replace() calls act on disjoint characters so neither re-escapes the other's output, and a"b / a'b / a'"b all round-trip) but if we can avoid it, that seems simpler.

[gordthompson]

Very helpful. Thanks!