On Fri, Dec 26, 2025 at 8:58 PM vignesh C <[email protected]> wrote: > > > Here is a rebased version of the remaining patches. >
Thank You Vignesh. Please find a few comments on 004: 1) IIUC, SubscriptionConflictrelIndexId is an unique index on sub-oid and conf-relid, but we use it only on relid as key. Why didn't we create it only on 'conf-relid' alone? Using a composite unique index is guaranteed to give unique row only when all keys are used, but for a single key, a unique row is not guaranteed. In our case, it will be a unique row as conflict-relid is not shared, but still as an overall general concept, it may not be. 2) IsConflictLogTable(): + if (OidIsValid(subform->subconflictlogrelid)) Do we need this check? Since we’ve already performed an index access using subconflictlogrelid as the key, isn’t it guaranteed to always be valid? 3) Please update the commit message to indicate that this patch makes CLT publishable if a publication is explicitly created on it, else few changes become very confusing due to unclear intent. 4) pg_relation_is_publishable(): /* Subscription conflict log tables are not published */ - result = is_publishable_class(relid, (Form_pg_class) GETSTRUCT(tuple)) && - !IsConflictLogTable(relid); Comment should be removed too. 5) We need to remove below comment: * Note: Conflict log tables are not publishable. However, we intentionally * skip this check here because this function is called for every change and * performing this check during every change publication is costly. To ensure * unpublishable entries are ignored without incurring performance overhead, * tuples inserted into the conflict log table uses the HEAP_INSERT_NO_LOGICAL * flag. This allows the decoding layer to bypass these entries automatically. */ bool is_publishable_relation(Relation rel) 6) get_rel_sync_entry: + /* is this relation used for conflict logging? */ + isconflictlogrel = IsConflictLogTable(relid); Shall we add a comment indicating the intent of change in this function. Something like: /* * Check whether this is a conflict log table. If so, avoid publishing it via * FOR ALL TABLES or FOR TABLES IN SCHEMA publications, but still allow it * to be published through a publication explicitly created for this table. */ thanks Shveta
