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?
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?
3)
+ /* Process EXCEPT table list */
+ if (relations != NIL)
+ {
+ Assert(rels != NIL);
+ PublicationAddTables(puboid, rels, true, NULL);
+ }
We can remove Assert from here too as we don't have it in the later
part of code where we use rels. I feel it is okay to not have Assert
as we are fetching rels in nearby logic only.
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?
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.
6)
I see a few error-messages mentioning EXCEPT 'clause' while others
mention EXCEPT 'list'. We can make the wording the same throughout.
7)
+ /*
+ * Check that the table is not part of any publication when changing to
+ * UNLOGGED, as UNLOGGED tables can't be published.
+ */
+ GetRelationPublications(RelationGetRelid(attachrel), NULL, &exceptpuboids);
+ if (exceptpuboids != NIL)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot attach table \"%s\" as partition because it is
referenced in a publication EXCEPT clause",
Comments need to be corrected. Copy/paste issue.
8)
tablesync.c:
+#include "replication/logical.h"
This inclusion is not needed now.
Reviewing further.
thanks
Shveta
