Thread
-
Re: Include schema-qualified names in publication error messages.
Peter Smith <smithpb2250@gmail.com> — 2026-05-27T06:20:05Z
On Thu, May 7, 2026 at 4:00 PM vignesh C <vignesh21@gmail.com> wrote: > ... > > Okay, then we can split the patch into two, the first patch to make > > the required changes only for EXCEPT, and the second one for the > > remaining pre-existing messages. We can push the first patch in HEAD > > and wait for some more opinions on the second one. > Hi Vignesh. The patch had previously been split for EXCEPT and non-EXCEPT changes. The 0001 patch was already pushed a while ago for PG19. I think now that 0002 patch can be revisited for PG20. ====== IMO, the logical replication messages should consistently always give fully qualified relation names in the error messages. The relation named in the message can sometimes be ambiguous when not schema qualified. I saw some previous comment from Euler [1] saying we should refrain from changing existing messages, but IMO here we are not rewording message text for the sake of it; I think rather it is fixing the values substituted to the *existing* messages to improve the clarity, and at the same time making all the logical replication logs more consistent. But, perhaps I misunderstood Euler's comment: if it was -1 referring only to backpatching then I agree. Anyway, I looked again at the old v5-0002 patch. I found it is only addressing the issue schema-qualification for check_publication_add_relation. Actually, I think that is just a very small part of something far bigger. e.g. There are many more places in logical replication related code where fully qualified names could be used. Also, sometimes the log message is already schema-qualified, but is not yet using the new "common" function designed for that purpose. Below are dozens of examples that I found by searching the pub/sub code for quoted relations/sequences/tables. Maybe you can find even more. ====== Relations: ------ src/backend/commands/publicationcmds.c TransformPubWhereClauses: errmsg("cannot use publication WHERE clause for relation \"%s\"", CheckPubRelationColumnList: errmsg("cannot use column list for relation \"%s.%s\" in publication \"%s\"", CheckPubRelationColumnList: errmsg("cannot use column list for relation \"%s.%s\" in publication \"%s\"", PublicationDropTables: errmsg("relation \"%s\" is not part of the publication", ------ src/backend/catalog/objectaddress.c get_object_address_attribute: errmsg("column \"%s\" of relation \"%s\" does not exist", get_object_address_attrdef: errmsg("default value for column \"%s\" of relation \"%s\" does not exist", get_object_address_publication_rel: errmsg("publication relation \"%s\" in publication \"%s\" does not exist", ------ src/backend/catalog/pg_publication.c check_publication_add_relation: errormsg = gettext_noop("cannot add relation \"%s\" to publication"); publication_add_relation: errmsg("relation \"%s\" is already member of publication \"%s\"", pub_collist_validate: errmsg("column \"%s\" of relation \"%s\" does not exist", ------ src/backend/catalog/pg_subscription.c RemoveSubscriptionRel: errdetail("Table synchronization for relation \"%s\" is in progress and is in state \"%c\".", ====== Sequences: ------ src/backend/commands/subscriptioncmds.c AlterSubscription_refresh: errmsg_internal("sequence \"%s.%s\" removed from subscription \"%s\"", AlterSubscription_refresh_seq: errmsg_internal("sequence \"%s.%s\" of subscription \"%s\" set to INIT state", ------ src/backend/replication/logical/sequencesync.c copy_sequences: "logical replication synchronization for subscription \"%s\", sequence \"%s.%s\" has finished", ====== Tables: ------ src/backend/commands/tablecmds.c ATPrepChangePersistence: errmsg("cannot change table \"%s\" to unlogged because it is part of a publication", ATExecAttachPartition: errmsg_plural("cannot attach table \"%s\" as partition because it is referenced in publication %s EXCEPT clause", ATExecAttachPartition: cannot attach table \"%s\" as partition because it is referenced in publications %s EXCEPT clause", ------ src/backend/commands/publicationcmds.c AlterPublicationOptions: errdetail("The publication contains a WHERE clause for partitioned table \"%s\", which is not allowed when \"%s\" is false.", AlterPublicationOptions: errdetail("The publication contains a column list for partitioned table \"%s\", which is not allowed when \"%s\" is false.", OpenTableList: errmsg("conflicting or redundant WHERE clauses for table \"%s\"", OpenTableList: errmsg("conflicting or redundant column lists for table \"%s\"", OpenTableList: errmsg("conflicting or redundant WHERE clauses for table \"%s\"", OpenTableList: errmsg("conflicting or redundant column lists for table \"%s\"", ------ src/backend/executor/execReplication.c CheckCmdReplicaIdentity: errmsg("cannot update table \"%s\"" CheckCmdReplicaIdentity: errmsg("cannot update table \"%s\"", CheckCmdReplicaIdentity: errmsg("cannot update table \"%s\"", CheckCmdReplicaIdentity: errmsg("cannot delete from table \"%s\"", CheckCmdReplicaIdentity: errmsg("cannot delete from table \"%s\"", CheckCmdReplicaIdentity: errmsg("cannot delete from table \"%s\"", - errmsg("cannot update table \"%s\" because it does not have a replica identity and publishes updates", - errmsg("cannot delete from table \"%s\" because it does not have a replica identity and publishes deletes", ------ src/backend/replication/logical/syncutils.c FinishSyncWorker: errmsg("logical replication table synchronization worker for subscription \"%s\", table \"%s\" has finished", ------ src/backend/replication/logical/worker.c InitializeLogRepWorker: errmsg("logical replication table synchronization worker for subscription \"%s\", table \"%s\" has started", ////// Regarding the code of v5-0002 patch, my comments are below: 1. Wasn't the newly pushed function called 'RelationGetQualifiedRelationName'. Maybe the name was changed after some time after your v5* patches? ~~~ 2. ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg(errormsg, relname), + errmsg(errormsg, get_relation_qualified_name(targetrel)), errdetail("This operation is not supported for individual partitions."))); That "get_relation_qualified_name(targetrel)" is repeated many times. IMO the code would be neater to just assign the name up-front to a variable. ====== [1] https://www.postgresql.org/message-id/d0979f9c-10a3-4983-8a41-7014135d02f9%40app.fastmail.com Kind Regards, Peter Smith. Fujitsu Australia