Re: Skipping schema changes in publication

On Tue, Mar 17, 2026 at 5:29 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Thanks for the fixes! Below are some more details about the un-fixed
> ones, in case it makes any difference.
>
> On Mon, Mar 16, 2026 at 9:56 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Mon, 16 Mar 2026 at 12:06, Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> ...
> > > 1b.
> > > Anyway, this note is talking about *subscriptions* to multiple
> > > publications, so I felt that it belongs in the CREATE SUBSCRIPTION
> > > "Notes" section. Or, maybe on both pages.
> >
> > I felt this talks about publication, so this place is ok. We can do it
> > like the way you are telling if others also feel that way.
>
> OK. Just so others are aware, the CREATE SUBSCRIPTION page "Notes"
> section [1] is already saying things like:
>
> i) "If the subscription has several publications in which the same
> table has been published with different WHERE clauses..."
>
> ii) "Subscriptions having several publications in which the same table
> has been published with different column lists..."
>
> So, all I am suggesting is that CREATE SUBSCRIPTION should also have
> exactly the same kind of worded note to describe behaviour for
> subscriptions having several publications with clashing
> included/excluded tables.
>

I don't know if that is required or not. Please wait to get the base
feature stabilized, we can add more docs later as well after some
inputs.

> > > ======
> > > src/backend/commands/tablecmds.c
> > >
> > > ATExecAttachPartition:
> > >
> > > 3.
> > > + bool first = true;
> > > + StringInfoData pubnames;
> > > +
> > > + initStringInfo(&pubnames);
> > > +
> > > + foreach_oid(pubid, exceptpuboids)
> > > + {
> > > + char    *pubname = get_publication_name(pubid, false);
> > > +
> > > + if (!first)
> > > + {
> > > + /*
> > > + * translator: This is a separator in a list of publication
> > > + * names.
> > > + */
> > > + appendStringInfoString(&pubnames, _(", "));
> > > + }
> > > +
> > > + first = false;
> > > +
> > > + appendStringInfo(&pubnames, _("\"%s\""), pubname);
> > > + }
> > >
> > > 3a.
> > > Why is appendStringInfo(&pubnames, _("\"%s\""), pubname) using the
> > > "_(" macro?. AFAIK it does not make sense to call gettext() for a
> > > pubname.
> >
> > I had seen that it is implemented similarly in logicalrep_get_attrs_str too:
> > appendStringInfo(&attsbuf, _("\"%s\""), remoterel->attnames[i]);
> >
> > Amit also has mentioned this at [1].
>
> OK, but AFAICT that logicalrep_get_attrs_str code is the original
> mistaken usage. Copying it just makes 2 mistakes.
>
> e.g. the purpose of using gettext (e.g. the "_(x)" macro) is for
> marking the string for i18n translation. But there's nothing even to
> translate here, it's just a format specifier in quotes.
>
> Also, the fact that logicalrep_get_attrs_str was the one-and-only
> usage of _(\"%s\") in the entire PG source indicates to me that it's
> dubious.
>

I am not a translation expert, so not sure but will consider this
input after taking care of the pending main patch.

> > > 3b.
> > > Postgres already has a function to make a CSV list of pubnames. Can't
> > > we build a list of pubnames here, then just call the common
> > > 'GetPublicationsStr' to get that as a CSV string. PSA a patch
> > > demonstrating what I mean.
> >
> >  The current implementation loops the publication only once. In the
> > approach you suggested first you will loop through pubids and get the
> > pubname then pass it to GetPublicationsStr and loop again. I felt the
> > existing is better.
>
> Sure. It's your call.
>
> FWIW, I think the extra looping is insignificant because:
> i) loops will be tiny (how many publications will be publishing the same table?)
>

I don't have a ready answer for this, the number could be large as well.

> ii) it is only used for an ERROR message; performance hardly matters.
>
> OTOH, using the common function implementation is <10 lines instead of
> >20 lines.
>

Hmm, this is another way to look at it but looping extra time over
publications to call existing function sounds redundant to me. I don't
want to argue what is the best way for this but if more people also
prefer this pattern then we can change it.

-- 
With Regards,
Amit Kapila.