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?
>
> 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.
>
Few more comments on v50-001:
9)
+ * pub1:
+ * FOR ALL TABLES EXCEPT (part1, part2)
+ * WITH (publish_via_partition_root = true)
This example atop check_publications_except_list() needs to be changed
as now we can not specify partitions in EXCEPT.
10)
In get_rel_sync_entry(), for the code under:
/*
* If this is a FOR ALL TABLES publication, pick the partition
* root and set the ancestor level accordingly.
*/
Earlier when 'if (pub->alltables)' was true, 'publish=true' was set
unconditionally, that means it will never go to the subsequent 'if
(!publish)' block. I think that subsequent block was implicitly meant
for a pub which is not for alltables. But now it will go to that block
even for all-tables if we set publish as false because ROOT was
excluded. This unnecessary work should be avoided. A pub can either be
ALL-TABLE (with or without EXCEPT) or schema-pub at a time, but not
both.
11)
get_rel_sync_entry():
+ GetRelationPublications(pub_relid, NULL, &exceptpubids);
+
+ if (!list_member_oid(exceptpubids, pub->oid))
+ publish = true;
+
+ list_free(exceptpubids);
We should add a brief comment as to why we are checking only the last
ancestor in exclude-list. How about this:
Check exclusion of top-most ancestor. Currently, only the top-most
ancestor can be added to the EXCEPT list, which means a partition will
not be published if its top-most ancestor is excluded.
<please rephrase if needed>
12) pg_dump dumped "ONLY" along with ROOT name in EXCEPT. I think it
should be avoided.
pg_dump output:
CREATE PUBLICATION pub4 FOR ALL TABLES EXCEPT TABLE (ONLY
public.tab_root) WITH (publish = 'insert, update, delete, truncate');
13)
pg_dump did not dump EXCEPT list for combination of allseq, all tables pub.
original:
CREATE PUBLICATION pub0 FOR ALL SEQUENCES, ALL TABLES EXCEPT (users,
departments);
postgres=# \dRp+ pub0
Owner | All tables | All sequences |
--------+------------+---------------
shveta | t | t |
Except tables:
"public.departments"
"public. Users"
pg_dump output:
CREATE PUBLICATION pub0 FOR ALL TABLES, ALL SEQUENCES WITH (publish =
'insert, update, delete, truncate');
~~
Reviewing further.
thanks
Shveta
