On Thu, Feb 26, 2026 at 3:22 PM shveta malik <[email protected]> wrote:
>
> On Thu, Feb 26, 2026 at 11:42 AM shveta malik <[email protected]> wrote:
> >
> > On Wed, Feb 25, 2026 at 10:43 PM Shlok Kyal <[email protected]>
> > wrote:
> > >
> > > Hi Shveta, Ashutosh, Chao-san,
> > >
> > > Thanks for reviewing the patch.
> > > I have addressed the above comments and the comments in [1] and [2].
> > > Attached is the latest v50 version.
> > >
> >
> > Thank You Shlok. Please find a few comments on v50-001.
> >
> > 1)
> > -check_publication_add_relation(Relation targetrel)
> > +check_publication_add_relation(PublicationRelInfo *pri, bool puballtables)
> >
> > Why do we need argument puballtables? I think we can give partition
> > related error even without checking 'puballtables', like we are giving
> > for temp and unlogged table error in EXCEPT list without checking
> > puballtables. Or let me know if I am missing the point here?
> >
One more point related to this part of code:
+ if (pri->except)
+ errormsg = gettext_noop("cannot use publication EXCEPT clause for
relation \"%s\"");
+ else
+ errormsg = gettext_noop("cannot add relation \"%s\" to publication");
+
+ /* If in EXCEPT clause, must be root partitioned table */
+ if (puballtables && pri->except && targetrel->rd_rel->relispartition)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(errormsg, RelationGetRelationName(targetrel)),
+ errdetail("This operation is not supported for individual partitions.")));
+
Why can't we directly use the required error message in errmsg?
> > 2)
> > publication_has_any_except_table(): Shall we optimize it:
> > a) if it is not all-table pub, return false there itself.
> > b) if it is all table, proceed with fetching tuples. At first tuple we
> > can break the loop irrespective of check 'pubrel->prexcept', as there
> > is no other feasibility. But we shall Assert (pubrel->prexcept) .
> > Thoughts?
> >
Hmm, let's not add such optimization checks here. It's not worth
making it specific to all_tables because tomorrow we will need to
support all_tables_in_schema case as well and we won't gain much with
such optimizations.
...
> >
> > 4)
> > CheckPublicationsForExceptClauses() frees except_publications list
> > only in case where it has to emit ERROR. It does not free it for a
> > case where there is only one element in it. Also one of the callers
> > free the list while another does not. Is it by design? Shall we make
> > the behaviour more consistent for callers?
> >
It should be consistent. I think in the ERROR case, most probably, it
will be freed by the containing memory context. I suggest check which
memory context's are used in all cases including when the
LoadPuclications() could be invoked via decoding APIs.
> > 5)
> > postgres=# ALTER TABLE tab_top_root ATTACH PARTITION tab_root FOR
> > VALUES FROM (0) TO (2000);
> > ERROR: cannot attach table "tab_root" as partition because it is
> > referenced in a publication EXCEPT clause
> > DETAIL: Tables excluded from publications cannot be attached as partitions.
> > HINT: Remove the table from the publication EXCEPT list before attaching
> > it.
> >
> > a) It will be good to list pubnames here for which we are referring EXCEPT
> > list.
> >
> > b) Also I feel, instead of emphasising on 'cannot attach table as
> > partition' in DETAIL, we should emphasise on 'partition cannot be part
> > of EXCEPT'. How about?
> > DETAIL: The publication EXCEPT clause cannot contain tables that are
> > partitions.
> >
+1.
--
With Regards,
Amit Kapila.
