Re: Skipping schema changes in publication

On Mon, 2 Mar 2026 at 16:18, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Sun, Mar 1, 2026 at 8:41 AM vignesh C <vignesh21(at)gmail(dot)com> 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=YNTnQzz2fcA@mail.gmail.com

Thanks,
Shlok Kyal