On Thu, Apr 30, 2026 at 1:02 PM vignesh C <[email protected]> wrote: > > On Wed, 29 Apr 2026 at 18:02, Dilip Kumar <[email protected]> wrote: > > > > On Wed, Apr 29, 2026 at 5:02 PM shveta malik <[email protected]> wrote: > > > > > > On Wed, Apr 29, 2026 at 4:41 PM Dilip Kumar <[email protected]> wrote: > > > > > > > > On Wed, Apr 29, 2026 at 4:08 PM Dilip Kumar <[email protected]> > > > > wrote: > > > > > > > > > > On Wed, Apr 29, 2026 at 9:28 AM Peter Smith <[email protected]> > > > > > wrote: > > > > > > > > > > > > On Tue, Apr 28, 2026 at 9:39 PM shveta malik > > > > > > <[email protected]> wrote: > > > > > > > > > > > > > > On Tue, Apr 28, 2026 at 4:34 PM Dilip Kumar > > > > > > > <[email protected]> wrote: > > > > > > > > > > > > > > > > Previously, error messages in check_publication_add_relation() > > > > > > > > only > > > > > > > > reported the relation name when a table could not be added to a > > > > > > > > publication or included in an EXCEPT clause. This could be > > > > > > > > ambiguous > > > > > > > > in databases where the same relation name exists in multiple > > > > > > > > schemas. > > > > > > > > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > This patch updates these error messages to use schema-qualified > > > > > > > > names, > > > > > > > > improving the clarity of error reporting for CREATE PUBLICATION > > > > > > > > and > > > > > > > > ALTER PUBLICATION commands. > > > > > > > > > > > > > > > > This has been discussed on another thread [1] > > > > > > > > > > > > > > > > > > > > > > The patch works well. > > > > > > > > > > > > > > I think we can pull out > > > > > > > 'get_namespace_name_or_temp(RelationGetNamespace(targetrel))' and > > > > > > > 'RelationGetRelationName(targetrel)' into local variables to > > > > > > > reduce > > > > > > > repetition and make the error paths a bit cleaner. > > > > > > > > > > > > > > const char *nspname = > > > > > > > get_namespace_name_or_temp(RelationGetNamespace(targetrel)); > > > > > > > const char *relname = RelationGetRelationName(targetrel); > > > > > > > > > > > > > > > > > > > How about having a dedicated function to return the fully qualified > > > > > > relation name you want, which can then substitute the single %s. > > > > > > > > > > > > e.g. > > > > > > errmsg(errormsg, get_qualified_relname(targetrel)), ... > > > > > > > > > > Yeah that makes sense. I will change this. > > > > > > > > > > One trivial thing: > > > +/* > > > + * Get a palloc'd string containing the schema-qualified name of the > > > relation. > > > + */ > > > > > > Extra space here: 'name of' > > > > Oops, fixed now. > > > > > There is a similar function, generate_qualified_relation_name(), > > > though I guess we can’t directly reuse it here. It takes a relid; > > > while we could extract the OID from the Relation and call it, that > > > seems a bit indirect when we already have the Relation in hand. In > > > that case, a local helper here seems reasonable. Right? > > > > Yeah, we already have a relation descriptor, so it's better to use that. > > We can remove the variables relname and result here by changing it to > something like: > static char * > get_qualified_relname(Relation rel) > { > char *nspname; > > nspname = get_namespace_name_or_temp(RelationGetNamespace(rel)); > if (!nspname) > elog(ERROR, "cache lookup failed for namespace %u", > RelationGetNamespace(rel)); > > return quote_qualified_identifier(nspname, > RelationGetRelationName(rel)); > }
Yeah we may, but I feel what we have now looks more readable. -- Regards, Dilip Kumar Google
