Hi Zsolt, Thanks a lot for the thoughtful review, you raised some great points.
I wonder if this could be simply an extension, similar to the > mentioned pgtrashcan, or even an update to that? > Yes, I think the mechanism can live largely in an extension, and that's the direction I'd like to take this for now. It keeps the footprint small, avoids touching core storage paths, and lets people opt in rather than paying for it everywhere. > extension, maybe the answer could be a different mechanism instead of > using one of the preexisting hooks trying to catch all possible > statements? I am not sure what that other mechanism could be, I just > wanted to point out that statement based tracking will always be > fragile. Since you are mostly worried about the data being destroyed > immediately, could be the solution file level instead? > > There's a patchset about making the smgr extensible [1] (I am one of > the authors/promoters of that patchset, and it is only slightly > related to this, but I do see a possible connection). Or regardless of > that patchset you could look into that area: making unlink a > soft-delete, so that even if a table is dropped/truncated, the data is > no longer automatically deleted. That would still require > reconstructing the catalog part of it, and non-destructive operations > such as vacuum full or rewriting alters would require special > handling. > I agree the file-level / soft-unlink approach via extensible smgr is a genuinely good alternative, and arguably the more robust one. It catches every path that destroys data rather than trying to enumerate statements, which as you rightly point out is inherently fragile. I'll follow that patchset with interest, and I can see a version of this feature built on top of it. For this round, though, I'd prefer to keep to the higher-level (catalog /statement) approach. The smgr route pulls in catalog reconstruction plus special handling for non-destructive rewrites (VACUUM FULL, rewriting ALTERs), which is a substantially larger effort and is coupled to a patchset that isn't committed yet. I'd rather land something useful and self-contained first, and treat the file-level mechanism as a possible future foundation once extensible smgr matures. > DROP TABLE ... CASCADE isn't the only CASCADE way to drop tables: > * DROP SCHEMA ... CASCADE > * DROP OWNED BY > * DROP EXTENSION ... CASCADE > * possibly others, isn't TRUNCATE similarly dangerous for example? Agreed, and I'll be explicit in the docs about which paths are covered rather than implying it catches everything. The object-drop paths (DROP SCHEMA/OWNED BY/EXTENSION ... CASCADE) all funnel through dependency-based deletion, so I'm hoping to hook at that lower level rather than per-statement, which should cover them more uniformly. TRUNCATE is a different codepath and I'll scope it out for now. > 1. interaction with other features, such as logical replication. If I > understand correctly, with the current WIP patch a dropped table would > remain a live relation, so it would stay in publications. Row level > security could be another concern. Most likely there are several more > similar corner cases. > > Good catch. A "trashed" relation staying in publications is not acceptable, so soft-dropped tables need to be removed from / excluded by publications (and similarly reconsidered for RLS). I'll work through these cases and document the behavior. > 2. security. Current api is public, when most likely it should be > completely revoked by default? > Yes, agreed. The API should be revoked from PUBLIC by default and only granted deliberately. I'll fix that. Regards, Surya Poondla
