Re: Proposal: Conflict log history table for Logical Replication

Please find few comments for patch-001 below -

1) Backend crash when conflict log table name already exists

+static Oid
+create_conflict_log_table(Oid subid, char *subname)
+{
+ TupleDesc tupdesc;
+ Oid relid;
+ ObjectAddress myself;
+ ObjectAddress subaddr;
+ char    relname[NAMEDATALEN];
+
+ snprintf(relname, NAMEDATALEN, "pg_conflict_%u", subid);
+
+ /* There can not be an existing table with the same name. */
+ Assert(!OidIsValid(get_relname_relid(relname, PG_CONFLICT_NAMESPACE)));

AFAIU, the above Assert is meant for catching exceptional cases, for
example when a user should never be able to create a table with the
same name.
But, currently, this scenario is easy to hit. For example:

postgres=# set allow_system_table_mods = on;
-- can create the table with same name as the conflict log table for the sub -
postgres=# create table pg_conflict.pg_conflict_16422(a int, b int);
CREATE TABLE

-- Now, try to switch the conflict_log_destination value to table or all
```
postgres=# alter subscription sub1 set (conflict_log_destination = all);
server closed the connection unexpectedly
        This probably means the server terminated abnormally
...
LOGs:
LOG:  background worker "logical replication tablesync worker" (PID
52471) exited with exit code 1
TRAP: failed Assert("!OidIsValid(get_relname_relid(relname,
PG_CONFLICT_NAMESPACE))"), File: "subscriptioncmds.c", Line: 3569,
PID: 51307
```
I’m not sure if we can strictly block CREATE TABLE in the pg_conflict
schema when allow_system_table_mods = on, since we don’t enforce
similar restrictions for other system schemas like pg_catalog,
pg_toast etc..
Perhaps replacing the Assert with a proper ERROR (with a helpful hint
about conflicting table names) would be better. Thoughts?
~~~

2)
+static Oid
+create_conflict_log_table(Oid subid, char *subname)
+{

The parameter "subname" is not used in this function; it can be removed.
~~~

3) A concern regarding permissions on the conflict log table.
>From earlier discussions, my understanding is that the subscription
owner (even if non-superuser) should be able to SELECT, DELETE, or
TRUNCATE their subscription’s conflict log table. But, this does not
seem to be working as expected.

I created a non-superuser and used it to create a subscription (after
granting the required permissions):

postgres=> select oid, subname, subowner, subconflictlogrelid,
subconflictlogdest from pg_subscription;
  oid  |  subname  | subowner | subconflictlogrelid | subconflictlogdest
-------+-----------+----------+---------------------+--------------------
 24697 | nisha_sub |    24621 |               24698 | table

-- Here, subowner (24621) is nisha, a non-superuser. She is also the
owner of the corresponding conflict log table:

postgres=# SELECT tableowner FROM pg_tables WHERE tablename =
'pg_conflict_24697';
 tableowner
------------
 nisha

But, nisha is not able to SELECT, DELETE, or TRUNCATE the table by default.
postgres=> select * from pg_conflict.pg_conflict_24697;
ERROR:  permission denied for schema pg_conflict
LINE 1: select * from pg_conflict.pg_conflict_24697;
                      ^
Currently, a non-superuser subscription owner cannot access their own
conflict log table unless a superuser manually grants the required
permissions.
Shouldn't create_conflict_log_table() grant USAGE on the pg_conflict
schema (or appropriate table privileges) to the subscription owner by
default? Please correct me if I missed something.
~~~

--
Thanks,
Nisha