tanmayrauth commented on code in PR #1466:
URL: https://github.com/apache/iceberg-go/pull/1466#discussion_r3592508128
##########
catalog/sql/sql.go:
##########
@@ -847,33 +887,33 @@ func (c *Catalog) DropNamespace(ctx context.Context,
namespace table.Identifier)
nsToDelete := strings.Join(namespace, ".")
- exists, err := c.namespaceExists(ctx, nsToDelete)
- if err != nil {
- return err
- }
-
- if !exists {
- return fmt.Errorf("%w: %s", catalog.ErrNoSuchNamespace,
nsToDelete)
- }
-
- tbls := make([]table.Identifier, 0)
- iter := c.ListTables(ctx, namespace)
-
- for tbl, err := range iter {
- tbls = append(tbls, tbl)
+ return withSerializableWriteTx(ctx, c.db, func(ctx context.Context, tx
bun.Tx) error {
Review Comment:
The serializable txn gives you the mutual exclusion, but the failure mode it
introduces isn't handled anywhere. On Postgres SERIALIZABLE one txn aborts with
SQLSTATE 40001 at commit; on MySQL SERIALIZABLE the two existence SELECTs
become LOCK IN SHARE MODE and the read-then-write pair deadlocks
(ER_LOCK_DEADLOCK); and on the default production SQLite catalog — which sets
neither WAL nor busy_timeout (those are only in the test) — two concurrent
writers get SQLITE_BUSY immediately. In all three cases
CreateTable/CreateView/DropNamespace/staged-CommitTable return that raw driver
error wrapped in none of the catalog sentinels, so a caller doing
errors.Is(err, table.ErrCommitFailed) or ErrNamespaceNotEmpty to retry never
matches and just sees an opaque BUSY/deadlock. That's inconsistent with the
update path at sql.go:625, which wraps its conflict in table.ErrCommitFailed
precisely so Table.doCommit retries. These new serializable paths should either
retry on the transient seriali
zation error or wrap it in a recognized sentinel.
##########
catalog/sql/sql.go:
##########
@@ -847,33 +887,33 @@ func (c *Catalog) DropNamespace(ctx context.Context,
namespace table.Identifier)
nsToDelete := strings.Join(namespace, ".")
- exists, err := c.namespaceExists(ctx, nsToDelete)
- if err != nil {
- return err
- }
-
- if !exists {
- return fmt.Errorf("%w: %s", catalog.ErrNoSuchNamespace,
nsToDelete)
- }
-
- tbls := make([]table.Identifier, 0)
- iter := c.ListTables(ctx, namespace)
-
- for tbl, err := range iter {
- tbls = append(tbls, tbl)
+ return withSerializableWriteTx(ctx, c.db, func(ctx context.Context, tx
bun.Tx) error {
+ objectsExist, err :=
tx.NewSelect().Model((*sqlIcebergTable)(nil)).
+ ColumnExpr("1").
+ Where("catalog_name = ?", c.name).
+ Where("table_namespace = ?", nsToDelete).
+ Limit(1).Exists(ctx)
if err != nil {
- return err
+ return fmt.Errorf("error checking namespace '%s' for
catalog objects: %w", namespace, err)
}
- break // there is already at least a table
- }
+ propsExist, err :=
tx.NewSelect().Model((*sqlIcebergNamespaceProps)(nil)).
+ ColumnExpr("1").
+ Where("catalog_name = ?", c.name).
+ Where("namespace = ?", nsToDelete).
+ Limit(1).Exists(ctx)
+ if err != nil {
+ return fmt.Errorf("error checking namespace '%s'
properties: %w", namespace, err)
Review Comment:
`namespace` here is a table.Identifier ([]string), so '%s' prints `namespace
'[foo]'` while the rest of the function uses the joined nsToDelete (`foo`). Use
nsToDelete in both messages so the error text is consistent and not bracketed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
