On Mon, 2 Mar 2026 at 16:18, Amit Kapila <[email protected]> wrote: > > On Sun, Mar 1, 2026 at 8:41 AM vignesh C <[email protected]> wrote: > > > > The attached v53 version patch has the changes for the same. > > > > Few comments on 0001: > =================== > 1. > + appendStringInfoString(&pubnames, quote_literal_cstr(pubname)); > + } > + > + ereport(ERROR, > + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > + errmsg_plural("cannot attach table \"%s\" as partition because it is > referenced in publication \"%s\" EXCEPT clause", > + "cannot attach table \"%s\" as partition because it is referenced > in publications \"%s\" EXCEPT clause", > > Because of the quoting before the error message, the publication names > in the error message will be displayed in single quotes which are > inside double quotes. > See below examples: > ERROR: cannot attach table "testpub_root" as partition because it is > referenced in publication "'testpub8'" EXCEPT clause > ERROR: cannot attach table "testpub_root" as partition because it is > referenced in publications "'testpub8', 'testpub10'" EXCEPT clause > > Do you this type of quoting single/multiple object names in error > messages at other places? What is the rationale behind this? > > The other place where I see such names is following but it also > doesn't use the pattern followed above: > A. > postgres=# create subscription sub1 connection 'dbname=postgres' > publication pub9, pub10, pub11; > WARNING: publications "pub9", "pub10", "pub11" do not exist on the publisher > postgres=# create subscription sub2 connection 'dbname=postgres' > publication pub12; > WARNING: publication "pub12" does not exist on the publisher > > B. > logical replication target relation "public.t10" is missing replicated > columns: "c2", "c3". For this See logicalrep_get_attrs_str and > following error_message > ereport(ERROR, > errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > errmsg_plural("logical replication target relation \"%s.%s\" is > missing replicated column: %s", > "logical replication target relation \"%s.%s\" is missing replicated > columns: %s", > > I think we should follow what is done in logicalrep_get_attrs_str for > message construction and translation. > I agree. Made the changes for the same.
> 2.
> * pub_all_obj_type is one of:
> *
> - * TABLES
> + * TABLES [EXCEPT [TABLE] ( table [, ...] )]
>
> Though the table is no longer optional, the above syntax reference
> still shows it as optional.
>
> 3.
> + List *ancestor_exceptpuboids =
> GetRelationExcludedPublications(ancestor);;
>
> One spurious semicolon at the end of above statement
>
> 4.
> @@ -5838,16 +5841,22 @@ RelationBuildPublicationDesc(Relation
> relation, PublicationDesc *pubdesc)
> foreach(lc, ancestors)
> {
> Oid ancestor = lfirst_oid(lc);
> + List *ancestor_puboids = GetRelationIncludedPublications(ancestor);
> + List *ancestor_exceptpuboids =
> GetRelationExcludedPublications(ancestor);;
>
> - puboids = list_concat_unique_oid(puboids,
> - GetRelationPublications(ancestor));
> + puboids = list_concat_unique_oid(puboids, ancestor_puboids);
> schemaid = get_rel_namespace(ancestor);
> puboids = list_concat_unique_oid(puboids,
> GetSchemaPublications(schemaid));
> + exceptpuboids = list_concat_unique_oid(exceptpuboids,
> + ancestor_exceptpuboids);
> }
>
> Why do we need to get the exceptpuboids for all ancestors instead of
> just the top-one as we are doing in get_rel_sync_entry()?
>
I agree we should only check the root partitioned table here. Modified the code
I have also addressed the other comments.
I have addressed the comments shared by Shveta in [1].
Attached the updated v54 patch.
[1]:
https://www.postgresql.org/message-id/CAJpy0uCr15=dxg+bmGeJUoNfKOHU2xZd2Wa6hg=yntnqzz2...@mail.gmail.com
Thanks,
Shlok Kyal
v54-0001-Skip-publishing-the-tables-specified-in-EXCEPT-T.patch
Description: Binary data
