Re: Proposal: Conflict log history table for Logical Replication

On Mon, 25 May 2026 at 07:07, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Vignesh,
>
> Some review comments for v38-0001/0002 combined.
>
> ======
> src/backend/commands/subscriptioncmds.c
>
> AlterSubscriptionConflictLogDestination:
>
> 1.
> * Update the conflict log table associated with a subscription when its
> * conflict log destination is changed.
>
> Somehow, that 'its' sounded awkward to me.
>
> SUGGESTION.
> When the subscription's 'conflict_log_destination' is changed, update
> the conflict log table if required.
>
> ~~~
>
> 2.
> + * If the new destination requires a conflict log table and none was previously
> + * required, this function validates an existing conflict log table identified
> + * by the subscription specific naming convention or creates a new one.
>
> What does this mean: "validates an existing conflict log table". How
> is there an "existing" CLT when you already said "none was previously
> required". Maybe this needs more explanation. If it is talking about
> "not already associated with another subscription", then it should
> just say that.
>
> Anyway, it seems validation that the comment claims this function is
> doing is not done here at all, but is really done by
> 'create_conflict_log_table'.
>
> ~~~
>
> 3.
> +static bool
> +AlterSubscriptionConflictLogDestination(Subscription *sub,
> + ConflictLogDest logdest,
> + Oid *conflicttablerelid)
>
> 3a.
> There was no forward declaration of this static function, but there
> was for all the others.
>
> ~
>
> 3b.
> Static functions should use snake-case names.
>
> ~~~
>
> 4.
> Personally, I think it is more natural to read LEFT-TO-RIGHT,
> OLD-THEN-NEW, etc., so I felt that the has_oldtable check should
> always come before want_table.
>
> Also, the 'ifs' seemed tricky because it's not obvious what
> has/need_table combinations are missing. e.g. The following seems
> easier to me. And probably lots of comments could be moved to here in
> the code as well, instead of in the function comment.
>
> SUGGESTION
> if (has_old_table)
> {
> /* There is a CLT already. */
>
> if (!want_table)
> {
> /* Remove it. */
> drop_subscription_dependencies(sub->oid, sub->name, sub->conflictlogrelid);
> update_relid = true;
> }
> }
> else
> {
> /* There was no previous CLT. */
>
> if (want_table)
> {
> /* Create one. */
> relid = create_conflict_log_table(sub->oid, sub->name, sub->owner);
> update_relid = true;
> }
> }
>
> ~~~
>
> 5.
> +static void
> +drop_subscription_dependencies(Oid subid, char *subname,
> + Oid subconflictlogrelid)
> +{
> + ObjectAddress object;
> + char *conflictrelname;
> +
> + conflictrelname = get_rel_name(subconflictlogrelid);
> +
> + /*
> + * By using PERFORM_DELETION_SKIP_ORIGINAL, we ensure that only the
> + * conflict log table is deleted while the subscription remains.
> + */
> + ObjectAddressSet(object, SubscriptionRelationId, subid);
> + performDeletion(&object, DROP_CASCADE,
> + PERFORM_DELETION_INTERNAL |
> + PERFORM_DELETION_SKIP_ORIGINAL);
> +
> + ereport(NOTICE,
> + errmsg("dropped conflict log table \"%s\" for subscription \"%s\"",
> + get_qualified_objname(PG_CONFLICT_NAMESPACE, conflictrelname),
> + subname));
> +}
> +
>
> One day, this function might do more than just remove the CLT, so IMO
> all this function body should be within a check:
>
> if (OidIsValid(subconflictlogrelid))
> {
> /* Drop any dependent CLT */
> ...
> }
>
> ~~~
>
> DropSubscription
>
> 6.
> + if (OidIsValid(subconflictlogrelid))
> + drop_subscription_dependencies(subid, subname, subconflictlogrelid);
>
> Make it unconditional. Instead, add the condition inside the
> 'drop_subscription_dependencies', per the previous review comment #5.
>
> ======
> src/test/regress/sql/subscription.sql
>
> 7.
> +--
> +-- PUBLICATION: Verify conflict log tables are not publishable
> +--
> +-- pg_relation_is_publishable should return false for internal conflict log
> +-- tables to prevent them from being accidentally included in publications
> +--
>
> Everywhere else, you had removed the word "internal", but this one
> (maybe others?) was missed.

Thanks for the comments, these are addressed in the v40 version patch attached.

Regards,
Vignesh