Re: Proposal: Conflict log history table for Logical Replication

Hello,

I want to come back to the DROP_CASCADE in drop_sub_conflict_log_table().
This was discussed in June, but I think the follow-up items from that
discussion got lost before the commit.

What I found in the thread:

* Shveta noticed that DROP SUBSCRIPTION also drops tables inheriting
from the CLT, because the CLT is removed with DROP_CASCADE [1].
* Dilip's position was that this kind of indirect usage is the user's
problem, as long as it is documented [2], and Shveta agreed, provided
that the docs get a CAUTION saying DROP SUBSCRIPTION cascades to the
CLT "and all its dependent objects, including any user-created
inherited tables, view etc" [3].
* Later Amit also said that it is fine to block views on the CLT for
the first version [4].

What is in master now:
inheritance is blocked, views are not, and create_subscription.sgml
only says that the table is dropped, there isn't anything about other
objects.
drop_subscription.sgml also still says that CASCADE/RESTRICT "do not
have any effect, since there are no dependencies on subscriptions",
which is no longer true, DROP SUBSCRIPTION ... RESTRICT cascades.

The June discussion only mentioned inherited tables and views, while
the cascade goes through everything that depends on the CLT or on its
row type.
I/Claude tried a bunch of object types, I think the realistic ones are:

* views and materialized views on the CLT, and views built on top of those
* SQL functions referencing the CLT in a BEGIN ATOMIC body, or
taking/returning its row type (and with them, functions calling these)
* maybe table columns typed as the CLT row type, if someone archives
conflict rows that way

All of these are silently dropped (there is a NOTICE with the list,
but no error) by DROP SUBSCRIPTION, and also by ALTER SUBSCRIPTION ...
SET (conflict_log_destination = log).
The ALTER case seems more surprising, as it looks like a logging setting change.
Constraints, indexes, triggers or policies on other tables are also
dropped if their expressions reference the row type, but I don't think
anybody would use those in practice.

Two more things I want to point out, which I think wasn't mentioned before:

1. The dropped objects don't have to belong to the subscription owner,
and the owner doesn't need any privilege on them. This is how I
originally found the issue, as my claude feature crosscheck classified
this as a security issue.

DROP ... CASCADE crosses ownership boundaries too, but there somebody
explicitly asks for it.
In this case nobody does (rather, it happens even with DROP
SUBSCRIPTION RESTRICT), and the person creating the monitoring views
and the subscription owner can easily be different people.

2. Because the deletion uses PERFORM_DELETION_INTERNAL, sql_drop event
triggers don't see the cascaded objects either,
pg_event_trigger_dropped_objects() only reports the subscription.

Options I see:

a. Do what was mentioned in June: document the cascade properly in
create/alter/drop_subscription, and fix the CASCADE/RESTRICT
paragraph.
b. Make DROP SUBSCRIPTION honor its existing CASCADE | RESTRICT: with
the default RESTRICT, error out if anything other than the CLT's own
objects depends on the CLT, the same way DROP TABLE does.
ALTER SUBSCRIPTION has no CASCADE, so there it could error out with a
hint to drop the dependent objects first. (b would need some
documentation changes too)
c. Block views/functions/... on the CLT, as Amit suggested, but that
seems hard to do completely, there are many ways to reference a table
or its row type.

So a or b seem more realistic to me, and b seems relatively easy to
implement to me: one additional arg to drop_sub_conflict_log_table
passed to performDeletion instead of a fixed DROP_CASCADE, and calling
it earlier in DropSubscription, and everything seems to work properly
in my local testing.

It would also match how other DROP commands behave, and it doesn't
break the monitoring use case which was a reason to allow views
outside pg_conflict [5].

[1]: https://postgr.es/m/CAJpy0uBhJGqD+OyA9Uk8bHyk61XWHEf3Le1QxkotwOLcQCqaZA@mail.gmail.com
[2]: https://postgr.es/m/CAFiTN-teHjcn7OkfE7n-e=sKGogeEjcZ+4HcfB2FaD1-55zDpg@mail.gmail.com
[3]: https://postgr.es/m/CAJpy0uDanE7J9hLOy0jnWJqXtUaERhiYkP5Ba9HJSkypK0Sn4A@mail.gmail.com
[4]: https://postgr.es/m/CAA4eK1JpOUo=3hwCYwkWyG8uJV+WcxBw3Y5SQpHgP1rUTt2QHg@mail.gmail.com
[5]: https://postgr.es/m/CAJpy0uDoa0CYWkxj52h=RM53acfsqjRihCfKrm8W=vRvHg01UA@mail.gmail.com