On Fri, 2 Jan 2026 at 12:06, shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, Dec 26, 2025 at 8:58 PM vignesh C <vignesh21@gmail.com> 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.
As we are searching only on subconflictlogrelid, index on
subconflictlogrelid is sufficient. I have modified it.
> 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?
It is not required, removed it
> 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.
Included this in the commit message.
> 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.
Removed
> 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)
Removed
> 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.
> */
Included
The attached v19 patch has the changes for the same.
Regards,
Vignesh
