On Tue, Jul 28, 2026 at 2:18 PM shveta malik <[email protected]> wrote:
>
> On Thu, Jul 23, 2026 at 5:26 PM Nisha Moond <[email protected]> wrote:
> >
> >
> > Attached v23 patch set addressing all of the above comments, as well
> > as the comments in [1] and [2].
> >
>
> A few comments on v23-001:
>
> 1)
> + if (CheckPublicationRelEntry(pubid, root, &root_except) && root_except)
> + ereport(ERROR,
> + errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("cannot add partition \"%s\" to publication \"%s\"",
> + RelationGetQualifiedRelationName(targetrel),
>
> Is the errorcode "ERRCODE_DUPLICATE_OBJECT" intentional? To me it does
> not look duplicate object as ROOT was in EXCEPT while this
> we are trying to add to inclusion list. Other similar places have
> different error, see:
>
> + if (exceptrelid == explicitrelid)
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("table \"%s\" cannot be both published and excluded",
> + RelationGetQualifiedRelationName(pri->relation)));
>
ERRCODE_INVALID_PARAMETER_VALUE seems like a better fit. Replaced it.
>
> 2)
> + * Also reject any relation in explicitrelids whose partition root is in
> + * except_rels. Otherwise the catalog would be inconsistent (root excluded,
> + * partition explicitly included), and the ancestor-cascade rule would
> + * silently override the include at replication time.
>
> Shall we simply say:
> /*
> * Also reject any relation in explicitrelids whose partition root is in
> * except_rels. Excluding a partition root means that the entire partition
> * tree is excluded, so a partition cannot be explicitly added to the
> * publication if its partition root is excluded.
> */
>
Done
> 3)
> Shall we change this too
> + errdetail("The table is currently named in the EXCEPT clause of the
> publication."),
> similar to:
> + errdetail("Partition root \"%s\" is named in the publication's
> EXCEPT clause for schema \"%s\".",
>
Done, now the error looks like -
ERROR: cannot add table "public.t1" to publication "p1"
DETAIL: The table is named in the publication's EXCEPT clause for
schema "public".
>
> 4)
> +
> + /*
> + * ancestors is NIL for a partition with a pending DETACH
> + * CONCURRENTLY.
> + */
> + if (ancestors != NIL)
> + root = llast_oid(ancestors);
>
> I see non-null ancestors check at some places while other places do it
> directly, see:
> + root = llast_oid(get_partition_ancestors(relid));
>
> is it intentional?
>
No, it was a miss; I corrected it at above and one more place in pgoutput.c
> 5)
> + list_free(ancestors);
>
> This is also done at some places, while missed at other places.
I checked the patch and list_free(ancestors) exists everywhere I
introduced the list.
Besides these, I noticed at least five existing places in HEAD where
the list isn't freed. Do you think it's worth fixing those in this
patch, or would it be better as a separate cleanup?
--
Thanks,
Nisha
