Re: Include schema-qualified names in publication error messages. - Mailing list pgsql-hackers

On Thu, Apr 30, 2026 at 1:02 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, 29 Apr 2026 at 18:02, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Wed, Apr 29, 2026 at 5:02 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Wed, Apr 29, 2026 at 4:41 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > >
> > > > On Wed, Apr 29, 2026 at 4:08 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > > >
> > > > > On Wed, Apr 29, 2026 at 9:28 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > > > > >
> > > > > > On Tue, Apr 28, 2026 at 9:39 PM shveta malik <shveta.malik@gmail.com> wrote:
> > > > > > >
> > > > > > > On Tue, Apr 28, 2026 at 4:34 PM Dilip Kumar <dilipbalaut@gmail.com> 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