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?
> 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.
