Include schema-qualified names in publication error messages.

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);

thanks
Shveta

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)), ...

======
Kind Regards,
Peter Smith.
Fujitsu Australia

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.

--
Regards,
Dilip Kumar
Google

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.

--
Regards,
Dilip Kumar
Google

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'

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?

thanks
Shveta

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.

--
Regards,
Dilip Kumar
Google

On Wed, Apr 29, 2026 at 6:01 PM 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.

+static char *get_qualified_relname(Relation rel);

This declaration seems unnecessary. We typically avoid adding one
unless it’s required due to a later definition being used earlier.
Other than that, the patch looks good.

thanks
Shveta

On Thu, Apr 30, 2026 at 9:47 AM shveta malik <shveta.malik@gmail.com> wrote:

On Wed, Apr 29, 2026 at 6:01 PM 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.

+static char *get_qualified_relname(Relation rel);

This declaration seems unnecessary. We typically avoid adding one
unless it’s required due to a later definition being used earlier.

I generally prefer to add the static declaration irrespective of the
order. I understand your point, but that isn't uniformly followed,
though I prefer to add it.

Other than that, the patch looks good.

Thanks.

--
Regards,
Dilip Kumar
Google

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));
}

Regards,
Vignesh

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

On Thu, Apr 30, 2026, at 4:37 AM, Dilip Kumar wrote:

Yeah we may, but I feel what we have now looks more readable.

My suggestion is that this function should be available in a central place.
That's not the only place that could use qualified schema and relation. If you
search for get_namespace_name_or_temp you will notice that this code path is
repeated in other parts of the code too (see ruleutils.c). It would be good if
we can have a common path for it. Maybe the signature has to be
get_qualified_relname(Oid) to accommodate other cases.

--
Euler Taveira
EDB https://www.enterprisedb.com/

On Thu, 30 Apr 2026 at 7:14 PM, Euler Taveira <euler@eulerto.com> wrote:

On Thu, Apr 30, 2026, at 4:37 AM, Dilip Kumar wrote:

Yeah we may, but I feel what we have now looks more readable.

My suggestion is that this function should be available in a central place.
That's not the only place that could use qualified schema and relation. If
you
search for get_namespace_name_or_temp you will notice that this code path
is
repeated in other parts of the code too (see ruleutils.c). It would be
good if
we can have a common path for it. Maybe the signature has to be
get_qualified_relname(Oid) to accommodate

IMHO it’s not a good idea to use Oid when you already have reldesc.

—
Dilip

On Thu, Apr 30, 2026 at 7:23 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, 30 Apr 2026 at 7:14 PM, Euler Taveira <euler@eulerto.com> wrote:

On Thu, Apr 30, 2026, at 4:37 AM, Dilip Kumar wrote:

Yeah we may, but I feel what we have now looks more readable.

My suggestion is that this function should be available in a central place.
That's not the only place that could use qualified schema and relation. If you
search for get_namespace_name_or_temp you will notice that this code path is
repeated in other parts of the code too (see ruleutils.c). It would be good if
we can have a common path for it. Maybe the signature has to be
get_qualified_relname(Oid) to accommodate

IMHO it’s not a good idea to use Oid when you already have reldesc.

+1.

I looked at other use cases of get_namespace_name_or_temp(), and there
doesn’t seem to be any case where we already have a Relation
descriptor. So this appears to be a unique scenario, and I feel adding
a new function here makes sense. If needed, ruleutils.c’s
generate_qualified_relation_name() could be moved to a common location
in a separate patch.

Thanks,
Shveta