Re: Proposal: Conflict log history table for Logical Replication - Mailing list pgsql-hackers

On Mon, Apr 27, 2026 at 8:55 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> 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?

Yeah, that makes sense. I admit that while handling this case I forgot
we have an option for 'allow_system_table_mods,' so logically we
cannot have an assert. Good finding, thanks.

> ~~~
>
> 2)
> +static Oid
> +create_conflict_log_table(Oid subid, char *subname)
> +{
>
> The parameter "subname" is not used in this function; it can be removed.

Yeah, right. But, now we are planning to print a NOTICE something like
"created conflict log table pg_conflict.tablename for subscription
subname" so we would be using this.

> ~~~
>
> 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, I will test this and fix it in the next version by tomorrow.

--
Regards,
Dilip Kumar
Google