Hi Vignesh. 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 <[email protected]> wrote: > > On Mon, 16 Mar 2026 at 12:06, Peter Smith <[email protected]> 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. > > ====== > > 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. > > 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?) 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. ====== [1] https://www.postgresql.org/docs/devel/sql-createsubscription.html Kind Regards Peter Smith. Fujitsu Australia
