On Sat, Feb 28, 2026 at 5:27 PM Amit Kapila <[email protected]> wrote:
>
> On Fri, Feb 27, 2026 at 8:44 PM Nisha Moond <[email protected]> wrote:
> >
> > On Fri, Feb 27, 2026 at 5:01 PM Nisha Moond <[email protected]>
> > wrote:
> > >
> > > Thanks for the patches, I am still testing the patches, but sharing a
> > > few initial review comments.
> > >
> >
> > I did few tests on v51 all patches, please find further comments -
> >
> > 1. Partition Related Behavior
> > -----------------------------
> > Setup:
> > CREATE TABLE t_part (id int) PARTITION BY RANGE (id);
> > CREATE TABLE t_part_p1 PARTITION OF t_part FOR VALUES FROM (0) TO (100);
> > CREATE TABLE t_part_p2 (id int);
> > ALTER TABLE t_part ATTACH PARTITION t_part_p2 FOR VALUES FROM (100) TO
> > (200);
> >
> > t_part
> > |_> t_part_p1
> > |_> t_part_p2
> >
> > Publication:
> > create publication pubp1 for ALL TABLES EXCEPT TABLE (t_part);
> > -- No data from all three tables is replicated, which is expected.
> >
> > 1a) DETACH t_part_p2 from parent
> > ALTER TABLE t_part DETACH PARTITION t_part_p2;
> >
> > When t_part_p2 is detached, it becomes a standalone table and is no
> > longer part of the EXCEPT hierarchy. But, replication does not start
> > automatically. It requires REFRESH PUBLICATION on the subscriber.
> > Should we add a warning/hint during DETACH command to inform users
> > that REFRESH PUBLICATION is required?
> > It may also help to update the related documentation.
> >
>
> This is required even when a regular (non-partitioned table) is
> created to let the corresponding entry populated in
> pg_subscription_rel. See docs[1] (REFRESH PUBLICATION ..).
>
> > ~~
> > 1b): Attach the partition again
> > ALTER TABLE t_part ATTACH PARTITION t_part_p2 FOR VALUES FROM (100) TO
> > (200);
> > When the partition is attached back, replication stops immediately as
> > it becomes part of the excluded parent again. No REFRESH PUBLICATION
> > is required on the subscriber. In this case, users may not realize
> > that replication has stopped. Should we consider adding a warning
> > during ATTACH in such scenarios?
> > ~~~~
> >
>
> This is worth considering but my opinion is that we don't need it as
> users should be aware of such behavior. For example, today, if a
> publication has published a root table say tab_root and one of its
> partitions is detached, we should stop that partition's replication
> but do we give WARNING for such cases?
>
I tried, we do not give WARNING for such cases during ATTACH/DETACH on
HEAD. I also feel that a warning for this EXCEPT case (and the
previous one) is not needed.
> > 2. Inherited Tables Behavior
> > Setup:
> > CREATE TABLE t_inh (id int);
> > CREATE TABLE t_inh_c1 (c1 int) INHERITS (t_inh);
> > CREATE TABLE t_inh_c2 (c2 int) INHERITS (t_inh);
> > t_inh
> > |_> t_inh_c1
> > |_> t_inh_c2
> >
> > Publication:
> > create publication pubi1 for ALL TABLES EXCEPT TABLE (t_inh);
> > --All three tables are not replicated.
> >
> > 2a): Remove child from parent
> > alter table t_inh_c1 NO INHERIT t_inh;
> >
> > \d+ t_inh shows:
> > Except Publications:
> > "pubi1"
> > Child tables: t_inh_c2
> >
> > But the publication still shows:
> > Except tables:
> > "public.t_inh"
> > "public.t_inh_c1"
> > "public.t_inh_c2"
> >
> > t_inh_c1 is no longer part of the excluded hierarchy, yet it remains
> > in the EXCEPT list and replication does not start for it.
> > This appears inconsistent and may be a bug.
> >
>
> I think after removal, it shouldn't have been shown here, so, we
> should see why it is happening. OTOH, won't replication requires user
> to perform REFRESH PUBLICATION as in the previous case?
>
> Few other comments:
> 1.
> --- a/src/backend/commands/subscriptioncmds.c
> +++ b/src/backend/commands/subscriptioncmds.c
> @@ -37,6 +37,7 @@
> #include "miscadmin.h"
> #include "nodes/makefuncs.h"
> #include "pgstat.h"
> +#include "replication/logical.h"
> #include "replication/logicallauncher.h"
> #include "replication/logicalworker.h"
> #include "replication/origin.h"
>
> There is no other change in this file, so do we even need this header
> included?
>
> 2.
> + foreach_oid(pubid, exceptpuboids)
> + {
> + char *pubname = get_publication_name(pubid, false);
> +
> + if (!first)
> + appendStringInfoString(&pubnames, ", ");
> +
> + first = false;
> +
> + appendStringInfoString(&pubnames, quote_literal_cstr(pubname));
> + }
> +
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot attach table \"%s\" as partition because it is
> referenced in publication \"%s\" EXCEPT clause",
> + RelationGetRelationName(attachrel), pubnames.data),
> + errdetail("The publication EXCEPT clause cannot contain tables that
> are partitions."),
> + errhint("Remove the table from the publication EXCEPT clause before
> attaching it."));
>
> A. As there could be multiple publications, so errmsg_plural form
> should be used, refer to other places.
> B. I think errhint should be removed from 0001 and added back by 0002
> where you can say how user can remove it by using SET CLAUSE
>
> [1] - https://www.postgresql.org/docs/devel/sql-altersubscription.html
>
> --
> With Regards,
> Amit Kapila.
