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

On Mon, May 4, 2026 at 10:51 AM shveta malik <shveta.malik@gmail.com> wrote:

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.

I think for HEAD, we can move the common part of
generate_qualified_relation_name() and get_qualified_relname() to a
common function that takes relname as input. We can probably move it
to lsyscache.c.

Now, we also need to decide whether to backpatch the relevant change
to back-branches. It seems we didn't get the bug-report yet but
clearly what we do currently is not correct. So, we should ideally
backpatch it and in the back branches we don't need to expose it.
OTOH, as it is reported and is not a big issue, so we can keep this as
a HEAD only change as well. If we want to keep this as a HEAD only
change then shall we wait for PG20 branch to open or go for current
HEAD itself? What do you and or others think on this matter?

--
With Regards,
Amit Kapila.

On Tue, May 5, 2026 at 10:28 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, May 4, 2026 at 10:51 AM shveta malik <shveta.malik@gmail.com> wrote:

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.

I think for HEAD, we can move the common part of
generate_qualified_relation_name() and get_qualified_relname() to a
common function that takes relname as input. We can probably move it
to lsyscache.c.

Now, we also need to decide whether to backpatch the relevant change
to back-branches. It seems we didn't get the bug-report yet but
clearly what we do currently is not correct. So, we should ideally
backpatch it and in the back branches we don't need to expose it.
OTOH, as it is reported and is not a big issue, so we can keep this as
a HEAD only change as well.

+1, it is a rare error-case scenario and it has not been reported so
far despite of change being present for a long time. So I think the
HEAD-only change is good.

If we want to keep this as a HEAD only
change then shall we wait for PG20 branch to open or go for current
HEAD itself? What do you and or others think on this matter?

I am fine with both. Again, a rare scenario that hasn't reported yet,
we can wait for the Pg20 branch to open.

thanks
Shveta

On Tue, May 5, 2026 at 10:27 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, May 4, 2026 at 10:51 AM shveta malik <shveta.malik@gmail.com> wrote:

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.

I think for HEAD, we can move the common part of
generate_qualified_relation_name() and get_qualified_relname() to a
common function that takes relname as input. We can probably move it
to lsyscache.c.

Done that, I think we need to pass nspid and relname.

Now, we also need to decide whether to backpatch the relevant change
to back-branches. It seems we didn't get the bug-report yet but
clearly what we do currently is not correct. So, we should ideally
backpatch it and in the back branches we don't need to expose it.
OTOH, as it is reported and is not a big issue, so we can keep this as
a HEAD only change as well. If we want to keep this as a HEAD only
change then shall we wait for PG20 branch to open or go for current
HEAD itself? What do you and or others think on this matter?

I think we should apply in PG19. Although back-patching isn't
critical, since we already have an opportunity to fix it in PG19, why
not push it early?

--
Regards,
Dilip Kumar
Google

On Tue, May 5, 2026 at 4:02 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Now, we also need to decide whether to backpatch the relevant change
to back-branches. It seems we didn't get the bug-report yet but
clearly what we do currently is not correct. So, we should ideally
backpatch it and in the back branches we don't need to expose it.
OTOH, as it is reported and is not a big issue, so we can keep this as
a HEAD only change as well. If we want to keep this as a HEAD only
change then shall we wait for PG20 branch to open or go for current
HEAD itself? What do you and or others think on this matter?

I think we should apply in PG19. Although back-patching isn't
critical, since we already have an opportunity to fix it in PG19, why
not push it early?

I also think we should push it for PG19 especially because the EXCEPT
feature increased the usage of relation names without schema-name in
error messages. However, as we are past feature freeze, I wanted to
know the opinion of others as well.

--
With Regards,
Amit Kapila.

On Tue, May 5, 2026, at 7:42 AM, Amit Kapila wrote:

On Tue, May 5, 2026 at 4:02 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Now, we also need to decide whether to backpatch the relevant change
to back-branches. It seems we didn't get the bug-report yet but
clearly what we do currently is not correct. So, we should ideally
backpatch it and in the back branches we don't need to expose it.
OTOH, as it is reported and is not a big issue, so we can keep this as
a HEAD only change as well. If we want to keep this as a HEAD only
change then shall we wait for PG20 branch to open or go for current
HEAD itself? What do you and or others think on this matter?

I think we should apply in PG19. Although back-patching isn't
critical, since we already have an opportunity to fix it in PG19, why
not push it early?

I also think we should push it for PG19 especially because the EXCEPT
feature increased the usage of relation names without schema-name in
error messages. However, as we are past feature freeze, I wanted to
know the opinion of others as well.

-1 for backpatching. These messages (without schema qualification) has been
like this since the beginning. The function was not introduced by fd366065e06a
and the proposed patch are changing existing messages as well. It is a good
idea to keep visible messages (WARNING, ERROR, FATAL, PANIC) consistent so as
not to break log analysis tools.

I would say the target is v20. However, as Amit said, the change to the EXCEPT
clause message might be important, so I suggest changing it; I would leave the
other messages for the RMT to decide.

PS> since we don't have a REL_19_STABLE branch yet, backpatch refers to v18 or
earlier.

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

On Tue, May 5, 2026 at 5:56 PM Euler Taveira <euler@eulerto.com> wrote:

On Tue, May 5, 2026, at 7:42 AM, Amit Kapila wrote:

On Tue, May 5, 2026 at 4:02 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Now, we also need to decide whether to backpatch the relevant change
to back-branches. It seems we didn't get the bug-report yet but
clearly what we do currently is not correct. So, we should ideally
backpatch it and in the back branches we don't need to expose it.
OTOH, as it is reported and is not a big issue, so we can keep this as
a HEAD only change as well. If we want to keep this as a HEAD only
change then shall we wait for PG20 branch to open or go for current
HEAD itself? What do you and or others think on this matter?

I think we should apply in PG19. Although back-patching isn't
critical, since we already have an opportunity to fix it in PG19, why
not push it early?

I also think we should push it for PG19 especially because the EXCEPT
feature increased the usage of relation names without schema-name in
error messages. However, as we are past feature freeze, I wanted to
know the opinion of others as well.

-1 for backpatching.

Agreed.

These messages (without schema qualification) has been
like this since the beginning. The function was not introduced by fd366065e06a
and the proposed patch are changing existing messages as well. It is a good
idea to keep visible messages (WARNING, ERROR, FATAL, PANIC) consistent so as
not to break log analysis tools.

I would say the target is v20. However, as Amit said, the change to the EXCEPT
clause message might be important, so I suggest changing it; I would leave the
other messages for the RMT to decide.

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.

--
With Regards,
Amit Kapila.