Copilot commented on code in PR #14493:
URL: https://github.com/apache/gravitino/pull/14493#discussion_r4438824507
##########
catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlSchemaOperations.java:
##########
@@ -144,6 +168,67 @@ public String generateDropDatabaseSql(String schema,
boolean cascade) {
return sqlBuilder.toString();
}
+ @Override
+ protected void dropDatabase(String schema, boolean cascade) {
+ if (!cascade) {
+ super.dropDatabase(schema, false);
+ return;
+ }
+ try (Connection connection = getConnection()) {
+ connection.setAutoCommit(false);
+ try {
+ // Keep existing relations stable while checking dependencies and
executing the drop.
+ try (PreparedStatement relations =
+ connection.prepareStatement(
+ "SELECT c.relname FROM pg_catalog.pg_class c JOIN
pg_catalog.pg_namespace n "
+ + "ON c.relnamespace = n.oid WHERE n.nspname = ? AND
c.relkind IN ('r', 'p') ORDER BY c.oid")) {
Review Comment:
Locking only existing ordinary and partitioned tables leaves the dependency
check vulnerable to concurrent DDL. For a source view `s.v AS SELECT 1`,
another transaction can create and commit `other.v AS SELECT * FROM s.v` after
validation but before the DROP; none of these locks prevents it, and CASCADE
then deletes the new external view. Use a synchronization strategy that also
stabilizes dependencies on other object kinds and accounts for newly created
objects, and cover this interleaving with a two-connection test.
##########
catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlSchemaOperations.java:
##########
@@ -40,12 +40,36 @@
import org.apache.gravitino.catalog.jdbc.config.JdbcConfig;
import org.apache.gravitino.catalog.jdbc.converter.JdbcExceptionConverter;
import org.apache.gravitino.catalog.jdbc.operation.JdbcDatabaseOperations;
+import org.apache.gravitino.catalog.jdbc.utils.JdbcConnectorUtils;
import org.apache.gravitino.exceptions.NoSuchSchemaException;
+import org.apache.gravitino.exceptions.NonEmptySchemaException;
import org.apache.gravitino.meta.AuditInfo;
/** Database operations for PostgreSQL. */
public class PostgreSqlSchemaOperations extends JdbcDatabaseOperations {
+ private static final String EXTERNAL_DEPENDENTS_SQL =
+ "WITH RECURSIVE dependents(classid, objid, objsubid) AS ( "
+ + " SELECT 'pg_catalog.pg_namespace'::regclass::oid, oid, 0 "
+ + " FROM pg_catalog.pg_namespace WHERE nspname = ? "
+ + " UNION "
+ + " SELECT next_object.classid, next_object.objid,
next_object.objsubid "
+ + " FROM dependents current_object "
+ + " CROSS JOIN LATERAL ( "
+ + " SELECT d.classid, d.objid, d.objsubid FROM
pg_catalog.pg_depend d "
+ + " WHERE d.refclassid = current_object.classid AND d.refobjid =
current_object.objid "
+ + " AND (current_object.objsubid = 0 OR d.refobjsubid =
current_object.objsubid) "
+ + " UNION "
+ + " SELECT d.refclassid, d.refobjid, d.refobjsubid FROM
pg_catalog.pg_depend d "
+ + " WHERE d.classid = current_object.classid AND d.objid =
current_object.objid "
+ + " AND d.objsubid = current_object.objsubid AND d.deptype =
'i' "
+ + " ) next_object "
+ + ") "
+ + "SELECT DISTINCT pg_catalog.pg_describe_object(d.classid, d.objid,
d.objsubid) AS dependent "
+ + "FROM dependents d "
+ + "CROSS JOIN LATERAL pg_catalog.pg_identify_object(d.classid,
d.objid, d.objsubid) info "
+ + "WHERE info.schema IS NOT NULL AND info.schema <> ?; ";
Review Comment:
With a sequence `source.seq` and an external column default using
`nextval('source.seq')`, this check passes and CASCADE silently removes the
external default. The default's `pg_attrdef` entry has no identified schema,
and its automatic dependency on the owning column is not followed by the
internal-only reverse traversal. Resolve schema-less subobjects through their
owning relation and add a default-only regression test.
##########
catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlSchemaOperations.java:
##########
@@ -40,12 +40,36 @@
import org.apache.gravitino.catalog.jdbc.config.JdbcConfig;
import org.apache.gravitino.catalog.jdbc.converter.JdbcExceptionConverter;
import org.apache.gravitino.catalog.jdbc.operation.JdbcDatabaseOperations;
+import org.apache.gravitino.catalog.jdbc.utils.JdbcConnectorUtils;
import org.apache.gravitino.exceptions.NoSuchSchemaException;
+import org.apache.gravitino.exceptions.NonEmptySchemaException;
import org.apache.gravitino.meta.AuditInfo;
/** Database operations for PostgreSQL. */
public class PostgreSqlSchemaOperations extends JdbcDatabaseOperations {
+ private static final String EXTERNAL_DEPENDENTS_SQL =
+ "WITH RECURSIVE dependents(classid, objid, objsubid) AS ( "
+ + " SELECT 'pg_catalog.pg_namespace'::regclass::oid, oid, 0 "
+ + " FROM pg_catalog.pg_namespace WHERE nspname = ? "
+ + " UNION "
+ + " SELECT next_object.classid, next_object.objid,
next_object.objsubid "
+ + " FROM dependents current_object "
+ + " CROSS JOIN LATERAL ( "
+ + " SELECT d.classid, d.objid, d.objsubid FROM
pg_catalog.pg_depend d "
+ + " WHERE d.refclassid = current_object.classid AND d.refobjid =
current_object.objid "
+ + " AND (current_object.objsubid = 0 OR d.refobjsubid =
current_object.objsubid) "
+ + " UNION "
+ + " SELECT d.refclassid, d.refobjid, d.refobjsubid FROM
pg_catalog.pg_depend d "
+ + " WHERE d.classid = current_object.classid AND d.objid =
current_object.objid "
+ + " AND d.objsubid = current_object.objsubid AND d.deptype =
'i' "
+ + " ) next_object "
+ + ") "
+ + "SELECT DISTINCT pg_catalog.pg_describe_object(d.classid, d.objid,
d.objsubid) AS dependent "
+ + "FROM dependents d "
+ + "CROSS JOIN LATERAL pg_catalog.pg_identify_object(d.classid,
d.objid, d.objsubid) info "
+ + "WHERE info.schema IS NOT NULL AND info.schema <> ?; ";
Review Comment:
The dependency walk also reaches a table's internally owned TOAST table and
index, whose reported schema is `pg_toast`. Consequently, even a contained
table such as `s.t(payload text)` makes this check reject the cascade despite
having no external user dependents. Exclude internal TOAST storage from the
schema-boundary check and add a successful contained-drop case with a toastable
column.
##########
catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlSchemaOperations.java:
##########
@@ -144,6 +168,67 @@ public String generateDropDatabaseSql(String schema,
boolean cascade) {
return sqlBuilder.toString();
}
+ @Override
+ protected void dropDatabase(String schema, boolean cascade) {
+ if (!cascade) {
+ super.dropDatabase(schema, false);
+ return;
+ }
+ try (Connection connection = getConnection()) {
+ connection.setAutoCommit(false);
+ try {
+ // Keep existing relations stable while checking dependencies and
executing the drop.
+ try (PreparedStatement relations =
+ connection.prepareStatement(
+ "SELECT c.relname FROM pg_catalog.pg_class c JOIN
pg_catalog.pg_namespace n "
+ + "ON c.relnamespace = n.oid WHERE n.nspname = ? AND
c.relkind IN ('r', 'p') ORDER BY c.oid")) {
+ relations.setString(1, schema);
+ try (ResultSet result = relations.executeQuery()) {
+ while (result.next()) {
+ JdbcConnectorUtils.executeUpdate(
+ connection,
+ "LOCK TABLE ONLY "
+ + quoteName(schema)
+ + "."
+ + quoteName(result.getString(1))
+ + " IN ACCESS EXCLUSIVE MODE");
Review Comment:
A non-superuser schema owner can cascade-drop contained tables owned by
another role, but these explicit locks require table privileges that schema
ownership does not grant. Previously valid contained drops therefore fail with
permission denied before validation. Preserve schema-owner drop permissions
when choosing the locking strategy, and add a regression with separate schema
and table owners.
--
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]
