I'm presenting this configuration option since at the moment I believe it's the most practical way for people to work around production issues they hit today.

Long term, if we separate JDBC catalog into a separate library (I forgot where we landed on where JDBC fits in in terms of the catalog discussion that's happened in the past) that library could define status codes for each common, industry-standard database so a user doesn't have to specify anything.

See https://www.youtube.com/watch?v=uAQVGd5zV4I&t=1323s for the discussion on catalogs in the future.

would be good to move this, as properties are defined in JdbcUtil

To be consistent, I would move this property in JdbcUtil as it's where all properties are located.

Maybe make it by default but also configurable (not necessarily in this PR). For instance, in ActiveMQ JDBC Store, I did some special handling specific to vendor (some SQL states are specific to vendor). I can include that in my PR (in preparation) working with multiple vendors.

I think it would be helpful to add some javadoc and document what those errors actually mean.

nit: the definition of that var should probably be moved to the top (before the non-static vars)

should this have JdbcCatalog.PROPERTY_PREFIX as a prefix like the other properties have?

I don't think so , looking at https://iceberg.apache.org/docs/nightly/jdbc/?h=jdbc#iceberg-jdbc-integration

The properties which are prefixed with "jdbc" look to be specifically for configuring the JDBC connection url, like user,password, ssl etc. These are standard options when establishing a JDBC connection.

What we're trying to do here is independent of the JDBC connection URL and more like a typical catalog property.

I do see that we have strict-mode and schema-version catalog properties which kind of break this pattern since those aren't relevant for the JDBC connection URL, but I'm not sure we want to keep following that since it does seem like the original intention of the prefix was pretty clear on having it be for the connection URL. cc @jbonofre if you have a take on this

My take on this (maybe I'm wrong) is that anything specific to JDBC catalog should be prefixed by jdbc, just to be sure we don't have conflict with properties with same name not related to JDBC Catalog.
As JDBC Connection is used in JDBC Catalog, I would prefix with jdbc for consistency. As it's user facing properties, it's important to be consistent here.

that anything specific to JDBC catalog should be prefixed by jdbc, just to be sure we don't have conflict with properties with same name not related to JDBC Catalog. As JDBC Connection is used in JDBC Catalog, I would prefix with jdbc for consistency. As it's user facing properties, it's important to be consistent here.

I think if we do this it's not really consistent with what we do with other catalogs. For example, REST, Glue, Nessie catalogs don't neccessarily prefix any of their custom configurations with "glue", "rest", or "nessie" respectively; configuring
the catalog configuration is just a matter of setting the property via <catalog_name>.<property>. I also don't think we need to worry about conflicts in property names across different catalogs; in the end we use reflection to load the catalog and it's up to the catalog implementation to respect whichever properties we want, so we just need to make sure the properties are well defined.

The docs seemed pretty clear to me about the original intention of the "jdbc" prefix for the JDBC catalog https://iceberg.apache.org/docs/nightly/jdbc/?h=jdbc#configurations is that it's specifically meant for configuring JDBC connections (username/port/db name that are in the URL itself).

retryable_status_codes is not configuring the JDBC connection URL.
It is a catalog level property which controls when the catalog should retry establishing an entirely new connection. That's why I think it should be namespaced as it's own property (same as any other property that we do for other catalogs).

I feel like that's actually more consistent behavior for a user; they can know that everything under jdbc is part of the actual connection URL, and everything outside of the JDBC prefix is a general catalog level property.

Fair enough. It makes sense indeed. But it also means that strict-mode and schema-version should not have been prefixed by jdbc then.
We can change that, but as it's user facing, it could be problematic.
I can work on a PR to deprecated jdbc prefix (not still supporting it) for strict-mode and schema-version.

But it also means that strict-mode and schema-version should not have been prefixed by jdbc then.
Right, I think they should not have been.

We can change that, but as it's user facing, it could be problematic.
I can work on a PR to deprecated jdbc prefix (not still supporting it) for strict-mode and schema-version

Sure, we can either deprecate the prefix or just leave as is. Since it's relatively early days for strict-mode/schema-version in the catalog (e.g. it'll practically take time for these paths to get exercised, as views get adopted) I think it makes most sense just to deprecate and correct it.

Maybe worth to add 40001 and 40P01 state to the list:

• 40001 is the code used by most databases to identify deadlocks
• 40P01 is a PostgreSQL specific code for deadlocks as well (see https://www.postgresql.org/docs/13/errcodes-appendix.html)

Deadlocks are a candidate to retry imho.

Sure it seems like 40001 is typically a transaction rollback prompted by a database's deadlock detector kicking in and essentially killing one of the operations.

I think I'll add 40001 since it can be safely retried and is a common status code across vendors.

I'm a bit more hesitant to add database vendor specific codes (like 40P01) to this default set, since it sets a precedent to be a dumping ground for many states. I'd advocate (at least for now) we keep this default set to be a common set, and users pass in custom codes for other cases.

40P01 can be provided by the user, if using PostgreSQL. So, OK to add 40001 state only.

Thanks to make it configuration (as discussed in the PR): it will allow users to define database specific states.

My take on this (maybe I'm wrong) is that anything specific to JDBC catalog should be prefixed by jdbc, just to be sure we don't have conflict with properties with same name not related to JDBC Catalog.
As JDBC Connection is used in JDBC Catalog, I would prefix with jdbc for consistency. As it's user facing properties, it's important to be consistent here.