On Tue, Jul 28, 2026 at 2:16 AM surya poondla <[email protected]> wrote: > > Thanks for the comments and suggestions. > > Let me first be clearer about the scope of the current patch, since I think > that is the source of some of the concern. > The WIP patch deliberately covers DROP TABLE only. That was meant as the > smallest useful increment to get the mechanism (reserved schema, OID-derived > naming, catalog of dropped objects, restore/purge APIs) reviewed, not as a > claim that all drop paths are protected. > I should have said so explicitly on the thread, and I will make that a > documented limitation rather than leaving it implied. >
Building features incrementally is fine, but doing it in a way that isn't extensible is not. Here, the hook you're using can't cover all the drop paths, so even as a contrib module it should be built on a hook in the right place. Consider either extending an existing hook to cover this case or proposing a new one unless you can present a way to address other use cases from the same hook in an acceptable way. >> >> AFAICU, the sql_drop event fires after the objects have already been >> removed from the catalogs, and event triggers can only observe the >> command or abort it with an error, not rewrite a DROP into something >> that keeps the relation around. So it would let us reliably record >> what was dropped across all paths (DROP TABLE, DROP SCHEMA ... >> CASCADE, DROP OWNED, cascades), but I don't see how it lets us >> preserve the table for a later restore. > > > Agreed. I looked at this and it matches what you describe: the object list is > collected in deleteObjectsInList() before the deletion loop, but > EventTriggerSQLDrop() itself does not fire until ProcessUtilitySlow(), by > which point the catalog rows are gone. > Combined with event triggers being observe-or-abort, I do not see a way to > preserve the relation from there. > It would give reliable auditing of what was dropped across all paths, which > is valuable in itself, but not recoverability. > Same conclusion for object_access_hook / OAT_DROP better timing, but that > itself cannot suppress the deletion of a table's indexes, TOAST relation and > owned sequences along with it.. > >> >> For actually preserving the data across all the paths, we can actually >> implement it inside the deletion machinery itself, i.e. teaching >> performDeletion to divert a table into a move-to-reserved-schema (when >> the feature is enabled) instead of a hard delete, rather than >> intercepting at the statement layer. That reuses the property that a >> relation in another schema is still fully valid and covers cascades >> for free. This needs much more analysis. >> > I think this is the right long-term shape, and I would frame it as an > extension of the current design rather than a replacement for it. > The reason the two converge is that the paths in question already share the > same machinery: > DROP TABLE -> RemoveRelations() -> performMultipleDeletions() > DROP SCHEMA/EXTENSION -> RemoveObjects() -> performMultipleDeletions() > DROP OWNED BY -> shdepDropOwned() -> performMultipleDeletions() > and all of them end at deleteOneObject() -> doDeletion() -> > heap_drop_with_catalog() for the relation itself. > So DROP SCHEMA ... CASCADE is not a separate kind of table drop; it reaches > the same place > with the table pulled in by the dependency walk instead of named directly. > > A couple of points: > 1. Everything above the interception point in my patch i.e the reserved > schema, the naming scheme, the catalog of dropped objects, the restore and > purge APIs, the protections is independent of where the drop is caught, so > moving the interception point later does not change this. > 2. The intermediate step is smaller than it looks: at the statement layer > these paths are all DropStmt (OBJECT_SCHEMA, OBJECT_EXTENSION) or > DropOwnedStmt, all visible to the same hook. The extension can enumerate the > affected tables and move them aside before letting the original statement > proceed, which covers the CASCADE forms Zsolt listed without core changes. > The residual gap is the internal and non-statement paths, and that is where > diverting in the deletion machinery is genuinely necessary. > I think it could be tricky to enumerate all affected tables this way from the current hook. You require a lot of additional functionality to do catalog lookups to find required objects and even then it is not clear how we can alter the schema of all required tables. -- With Regards, Amit Kapila.
