Logical Replication - improve error message while adding tables to the publication in check_publication_add_relation

Hi,

While providing thoughts on [1], I observed that the error messages
that are emitted while adding foreign, temporary and unlogged tables
can be improved a bit from the existing [2] to [3]. For instance, the
existing message when foreign table is tried to add into the
publication "f1" is not a table" looks odd. Because it says that the
foreign table is not a table at all.

Attaching a small patch. Thoughts?

[1] - https://www.postgresql.org/message-id/CALj2ACWAxO3vSToT0o5nXL%3Drz5cNx90zaV-at%3DcvM14Tag4%3DcQ%40mail.gmail.com
[2] - t1 is a temporary table:
postgres=# CREATE PUBLICATION testpub FOR TABLE t1;
ERROR: table "t1" cannot be replicated
DETAIL: Temporary and unlogged relations cannot be replicated.

t1 is an unlogged table:
postgres=# CREATE PUBLICATION testpub FOR TABLE t1;
ERROR: table "t1" cannot be replicated
DETAIL: Temporary and unlogged relations cannot be replicated.

f1 is a foreign table:
postgres=# CREATE PUBLICATION testpub FOR TABLE f1;
ERROR: "f1" is not a table
DETAIL: Only tables can be added to publications.

[3] - t1 is a temporary table:
postgres=# CREATE PUBLICATION testpub FOR TABLE t1;
ERROR: temporary table "t1" cannot be replicated
DETAIL: Temporary, unlogged and foreign relations cannot be replicated.

t1 is an unlogged table:
postgres=# CREATE PUBLICATION testpub FOR TABLE t1;
ERROR: unlogged table "t1" cannot be replicated
DETAIL: Temporary, unlogged and foreign relations cannot be replicated.

f1 is a foreign table:
postgres=# CREATE PUBLICATION testpub FOR TABLE f1;
ERROR: foreign table "f1" cannot be replicated
DETAIL: Temporary, unlogged and foreign relations cannot be replicated.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Mar 10, 2021 at 10:44 AM Bharath Rupireddy <
bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:

> Hi,
>
> While providing thoughts on [1], I observed that the error messages
> that are emitted while adding foreign, temporary and unlogged tables
> can be improved a bit from the existing [2] to [3].
>

+1 for improving the error messages here.

> Attaching a small patch. Thoughts?
>

I had a look at the patch and it looks good to me. However, I think after
you have added the specific kind of table type in the error message itself,
now the error details seem to be giving redundant information, but others
might
have different thoughts.

The patch itself looks good otherwise. Also the make check and postgres_fdw
check looking good.

Regards,
Jeevan Ladhe

On Wed, Mar 10, 2021 at 1:27 PM Jeevan Ladhe
<jeevan(dot)ladhe(at)enterprisedb(dot)com> wrote:
>
> On Wed, Mar 10, 2021 at 10:44 AM Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>>
>> Hi,
>>
>> While providing thoughts on [1], I observed that the error messages
>> that are emitted while adding foreign, temporary and unlogged tables
>> can be improved a bit from the existing [2] to [3].
>
> +1 for improving the error messages here.

Thanks for taking a look at the patch.

>> Attaching a small patch. Thoughts?
>
> I had a look at the patch and it looks good to me. However, I think after
> you have added the specific kind of table type in the error message itself,
> now the error details seem to be giving redundant information, but others might
> have different thoughts.

The error detail is to give a bit of information of what and all
relation types are unsupported with the create publication statement.
But with the error message now showing up the type of relation, the
detail message looks redundant to me as well. If agreed, I can remove
that. Thoughts?

> The patch itself looks good otherwise. Also the make check and postgres_fdw
> check looking good.

Thanks.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Mar 10, 2021, at 2:14 AM, Bharath Rupireddy wrote:
> While providing thoughts on [1], I observed that the error messages
> that are emitted while adding foreign, temporary and unlogged tables
> can be improved a bit from the existing [2] to [3]. For instance, the
> existing message when foreign table is tried to add into the
> publication "f1" is not a table" looks odd. Because it says that the
> foreign table is not a table at all.
I wouldn't mix [regular|partitioned|temporary|unlogged] tables with foreign
tables. Although, they have a pg_class entry in common, foreign tables aren't
"real" tables (external storage); they even have different DDLs to handle it
(CREATE TABLE x CREATE FOREIGN TABLE).

postgres=# CREATE PUBLICATION testpub FOR TABLE f1;
ERROR: "f1" is not a table
DETAIL: Only tables can be added to publications.

I agree that "f1 is not a table" is a confusing message at first because
foreign table has "table" as description. Maybe if we apply the negation in
both messages it would be clear (using the same wording as system tables).

ERROR: "f1" is a foreign table
DETAIL: Foreign tables cannot be added to publications.

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

On Wed, Mar 10, 2021 at 5:48 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> On Wed, Mar 10, 2021, at 2:14 AM, Bharath Rupireddy wrote:
>
> While providing thoughts on [1], I observed that the error messages
> that are emitted while adding foreign, temporary and unlogged tables
> can be improved a bit from the existing [2] to [3]. For instance, the
> existing message when foreign table is tried to add into the
> publication "f1" is not a table" looks odd. Because it says that the
> foreign table is not a table at all.
>
> I wouldn't mix [regular|partitioned|temporary|unlogged] tables with foreign
> tables. Although, they have a pg_class entry in common, foreign tables aren't
> "real" tables (external storage); they even have different DDLs to handle it
> (CREATE TABLE x CREATE FOREIGN TABLE).
>
> postgres=# CREATE PUBLICATION testpub FOR TABLE f1;
> ERROR: "f1" is not a table
> DETAIL: Only tables can be added to publications.
>
> I agree that "f1 is not a table" is a confusing message at first because
> foreign table has "table" as description. Maybe if we apply the negation in
> both messages it would be clear (using the same wording as system tables).
>
> ERROR: "f1" is a foreign table
> DETAIL: Foreign tables cannot be added to publications.

Thanks. Changed the error message and detail to the way we have it for
system tables presently. Attaching v2 patch for further review.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Mar 11, 2021 at 8:26 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Wed, Mar 10, 2021 at 5:48 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:
> >
> > On Wed, Mar 10, 2021, at 2:14 AM, Bharath Rupireddy wrote:
> >
> > While providing thoughts on [1], I observed that the error messages
> > that are emitted while adding foreign, temporary and unlogged tables
> > can be improved a bit from the existing [2] to [3]. For instance, the
> > existing message when foreign table is tried to add into the
> > publication "f1" is not a table" looks odd. Because it says that the
> > foreign table is not a table at all.
> >
> > I wouldn't mix [regular|partitioned|temporary|unlogged] tables with foreign
> > tables. Although, they have a pg_class entry in common, foreign tables aren't
> > "real" tables (external storage); they even have different DDLs to handle it
> > (CREATE TABLE x CREATE FOREIGN TABLE).
> >
> > postgres=# CREATE PUBLICATION testpub FOR TABLE f1;
> > ERROR: "f1" is not a table
> > DETAIL: Only tables can be added to publications.
> >
> > I agree that "f1 is not a table" is a confusing message at first because
> > foreign table has "table" as description. Maybe if we apply the negation in
> > both messages it would be clear (using the same wording as system tables).
> >
> > ERROR: "f1" is a foreign table
> > DETAIL: Foreign tables cannot be added to publications.
>
> Thanks. Changed the error message and detail to the way we have it for
> system tables presently. Attaching v2 patch for further review.

Here's the v3 patch rebased on the latest master.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Mar 26, 2021 at 9:25 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> Here's the v3 patch rebased on the latest master.

Here's the v4 patch reabsed on the latest master, please review it further.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 5, 2021, at 12:27 AM, Bharath Rupireddy wrote:
> On Fri, Mar 26, 2021 at 9:25 AM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com <mailto:bharath.rupireddyforpostgres%40gmail.com>> wrote:
> > Here's the v3 patch rebased on the latest master.
>
> Here's the v4 patch reabsed on the latest master, please review it further.
/* UNLOGGED and TEMP relations cannot be part of publication. */
if (!RelationIsPermanent(targetrel))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("table \"%s\" cannot be replicated",
- RelationGetRelationName(targetrel)),
- errdetail("Temporary and unlogged relations cannot be replicated.")));
+ {
+ if (RelationUsesLocalBuffers(targetrel))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" is a temporary table",
+ RelationGetRelationName(targetrel)),
+ errdetail("Temporary tables cannot be added to publications.")));
+ else if (targetrel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" is an unlogged table",
+ RelationGetRelationName(targetrel)),
+ errdetail("Unlogged tables cannot be added to publications.")));
+ }

RelationIsPermanent(), RelationUsesLocalBuffers(), and
targetrel->rd_rel->relpersistence all refers to relpersistence. Hence, it is
not necessary to test !RelationIsPermanent().

I would slightly rewrite the commit message to something like:

Improve publication error messages

Adding a foreign table into a publication prints an error saying "foo is not a
table". Although, a foreign table is not a regular table, this message could
possibly confuse users. Provide a suitable error message according to the
object class (table vs foreign table). While at it, separate unlogged/temp
table error message into 2 messages.

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

On Mon, Apr 5, 2021 at 6:41 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:
> Here's the v4 patch reabsed on the latest master, please review it further.
>
> /* UNLOGGED and TEMP relations cannot be part of publication. */
> if (!RelationIsPermanent(targetrel))
> - ereport(ERROR,
> - (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> - errmsg("table \"%s\" cannot be replicated",
> - RelationGetRelationName(targetrel)),
> - errdetail("Temporary and unlogged relations cannot be replicated.")));
> + {
> + if (RelationUsesLocalBuffers(targetrel))
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("\"%s\" is a temporary table",
> + RelationGetRelationName(targetrel)),
> + errdetail("Temporary tables cannot be added to publications.")));
> + else if (targetrel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("\"%s\" is an unlogged table",
> + RelationGetRelationName(targetrel)),
> + errdetail("Unlogged tables cannot be added to publications.")));
> + }
>
> RelationIsPermanent(), RelationUsesLocalBuffers(), and
> targetrel->rd_rel->relpersistence all refers to relpersistence. Hence, it is
> not necessary to test !RelationIsPermanent().

Done.

> I would slightly rewrite the commit message to something like:
>
> Improve publication error messages
>
> Adding a foreign table into a publication prints an error saying "foo is not a
> table". Although, a foreign table is not a regular table, this message could
> possibly confuse users. Provide a suitable error message according to the
> object class (table vs foreign table). While at it, separate unlogged/temp
> table error message into 2 messages.

Thanks for the better wording.

Attaching v5 patch, please have a look.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 5, 2021 at 7:19 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Mon, Apr 5, 2021 at 6:41 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:
> > Here's the v4 patch reabsed on the latest master, please review it further.
> >
> > /* UNLOGGED and TEMP relations cannot be part of publication. */
> > if (!RelationIsPermanent(targetrel))
> > - ereport(ERROR,
> > - (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > - errmsg("table \"%s\" cannot be replicated",
> > - RelationGetRelationName(targetrel)),
> > - errdetail("Temporary and unlogged relations cannot be replicated.")));
> > + {
> > + if (RelationUsesLocalBuffers(targetrel))
> > + ereport(ERROR,
> > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("\"%s\" is a temporary table",
> > + RelationGetRelationName(targetrel)),
> > + errdetail("Temporary tables cannot be added to publications.")));
> > + else if (targetrel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("\"%s\" is an unlogged table",
> > + RelationGetRelationName(targetrel)),
> > + errdetail("Unlogged tables cannot be added to publications.")));
> > + }
> >
> > RelationIsPermanent(), RelationUsesLocalBuffers(), and
> > targetrel->rd_rel->relpersistence all refers to relpersistence. Hence, it is
> > not necessary to test !RelationIsPermanent().
>
> Done.
>
> > I would slightly rewrite the commit message to something like:
> >
> > Improve publication error messages
> >
> > Adding a foreign table into a publication prints an error saying "foo is not a
> > table". Although, a foreign table is not a regular table, this message could
> > possibly confuse users. Provide a suitable error message according to the
> > object class (table vs foreign table). While at it, separate unlogged/temp
> > table error message into 2 messages.
>
> Thanks for the better wording.
>
> Attaching v5 patch, please have a look.
>

We get the following error while adding an index:
create publication mypub for table idx_t1;
ERROR: "idx_t1" is an index

This error occurs internally from table_openrv function call, if we
could replace this with relation_openrv and then check the table kind,
we could throw a similar error message here too like the other changes
in the patch.

Regards,
Vignesh

On Wed, May 26, 2021 at 7:38 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > Attaching v5 patch, please have a look.
>
> We get the following error while adding an index:
> create publication mypub for table idx_t1;
> ERROR: "idx_t1" is an index
>
> This error occurs internally from table_openrv function call, if we
> could replace this with relation_openrv and then check the table kind,
> we could throw a similar error message here too like the other changes
> in the patch.

Do you say that we replace table_open in publication_add_relation with
relation_open and have the "\"%s\" is an index" or "\"%s\" is a
composite type" checks in check_publication_add_relation? If that is
so, I don't think it's a good idea to have the extra code in
check_publication_add_relation and I would like it to be the way it is
currently.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, May 26, 2021 at 7:55 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Wed, May 26, 2021 at 7:38 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > Attaching v5 patch, please have a look.
> >
> > We get the following error while adding an index:
> > create publication mypub for table idx_t1;
> > ERROR: "idx_t1" is an index
> >
> > This error occurs internally from table_openrv function call, if we
> > could replace this with relation_openrv and then check the table kind,
> > we could throw a similar error message here too like the other changes
> > in the patch.
>
> Do you say that we replace table_open in publication_add_relation with
> relation_open and have the "\"%s\" is an index" or "\"%s\" is a
> composite type" checks in check_publication_add_relation? If that is
> so, I don't think it's a good idea to have the extra code in
> check_publication_add_relation and I would like it to be the way it is
> currently.

Before calling check_publication_add_relation, we will call
OpenTableList to get the list of relations. In openTableList we don't
include the errordetail for the failure like you have fixed it in
check_publication_add_relation. When a user tries to add index objects
or composite types, the error will be thrown earlier itself. I didn't
mean to change check_publication_add_relation, I meant to change
table_openrv to relation_openrv in OpenTableList and include error
details in case of failure like the change attached. If you are ok,
please include the change in your patch.

Regards,
Vignesh

On Thu, May 27, 2021 at 9:02 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > Do you say that we replace table_open in publication_add_relation with
> > relation_open and have the "\"%s\" is an index" or "\"%s\" is a
> > composite type" checks in check_publication_add_relation? If that is
> > so, I don't think it's a good idea to have the extra code in
> > check_publication_add_relation and I would like it to be the way it is
> > currently.
>
> Before calling check_publication_add_relation, we will call
> OpenTableList to get the list of relations. In openTableList we don't
> include the errordetail for the failure like you have fixed it in
> check_publication_add_relation. When a user tries to add index objects
> or composite types, the error will be thrown earlier itself. I didn't
> mean to change check_publication_add_relation, I meant to change
> table_openrv to relation_openrv in OpenTableList and include error
> details in case of failure like the change attached. If you are ok,
> please include the change in your patch.

I don't think we need to change that. General intuition is that with
CREATE PUBLICATION ... FOR TABLE/FOR ALL TABLES one can specify only
tables and if at all an index/composite type is specified, the error
messages ""XXXX" is an index"/""XXXX" is a composite type" can imply
that they are not supported with CREATE PUBLICATION. There's no need
for a detailed error message saying "Index/Composite Type cannot be
added to publications.". Whereas foreign/unlogged/temporary/system
tables are actually tables, and we don't support them. So a detailed
error message here can state that explicitly.

I'm not taking the patch, attaching v5 again here to make cfbot happy
and for further review.

BTW, when we use relation_openrv, we have to use relation_close.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 27, 2021 at 10:28 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> I'm not taking the patch, attaching v5 again here to make cfbot happy
> and for further review.

Attaching v6 patch rebased onto the latest master.

Regards,
Bharath Rupireddy.

On Wed, Jul 7, 2021 at 5:35 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> Attaching v6 patch rebased onto the latest master.

I came across a recent commit 81d5995 and have used the same error
message for temporary and unlogged tables. Also added, test cases to
cover these error cases for foreign, temporary, unlogged and system
tables with CREATE PUBLICATION command. PSA v7.

commit 81d5995b4b78017ef9e5c6f151361d1fb949924c
Author: Peter Eisentraut <peter(at)eisentraut(dot)org>
Date: Wed Jul 21 07:40:05 2021 +0200

More improvements of error messages about mismatching relkind

Regards,
Bharath Rupireddy.

> On 26 Jul 2021, at 09:33, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:

> PSA v7.

This patch no longer applies on top of HEAD, please submit a rebased version.

--
Daniel Gustafsson https://vmware.com/

On Wed, Nov 3, 2021 at 6:21 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
> > On 26 Jul 2021, at 09:33, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> > PSA v7.
>
> This patch no longer applies on top of HEAD, please submit a rebased version.

Here's a rebased v8 patch. Please review it.

Regards,
Bharath Rupireddy.

> On 4 Nov 2021, at 05:24, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Wed, Nov 3, 2021 at 6:21 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>>
>>> On 26 Jul 2021, at 09:33, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>>
>>> PSA v7.
>>
>> This patch no longer applies on top of HEAD, please submit a rebased version.
>
> Here's a rebased v8 patch. Please review it.

This improves the user experience by increasing the granularity of error
reporting, and maps well with the precedent set in 81d5995b4. I'm marking this
Ready for Committer and will go ahead and apply this unless there are
objections.

--
Daniel Gustafsson https://vmware.com/

On Fri, Nov 12, 2021, at 9:41 AM, Daniel Gustafsson wrote:
> > On 4 Nov 2021, at 05:24, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >
> > On Wed, Nov 3, 2021 at 6:21 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> >>
> >>> On 26 Jul 2021, at 09:33, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >>
> >>> PSA v7.
> >>
> >> This patch no longer applies on top of HEAD, please submit a rebased version.
> >
> > Here's a rebased v8 patch. Please review it.
>
> This improves the user experience by increasing the granularity of error
> reporting, and maps well with the precedent set in 81d5995b4. I'm marking this
> Ready for Committer and will go ahead and apply this unless there are
> objections.
Shouldn't we modify errdetail_relkind_not_supported() to include relpersistence
as a 2nd parameter and move those messages to it? I experiment this idea with
the attached patch. The idea is to provide a unique function that reports
accurate detail messages.

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

On Sat, Nov 13, 2021 at 12:06 AM Euler Taveira <euler(at)eulerto(dot)com> wrote:
> > Here's a rebased v8 patch. Please review it.
>
> This improves the user experience by increasing the granularity of error
> reporting, and maps well with the precedent set in 81d5995b4. I'm marking this
> Ready for Committer and will go ahead and apply this unless there are
> objections.
>
> Shouldn't we modify errdetail_relkind_not_supported() to include relpersistence
> as a 2nd parameter and move those messages to it? I experiment this idea with
> the attached patch. The idea is to provide a unique function that reports
> accurate detail messages.

Thanks. It is a good idea to use errdetail_relkind_not_supported. I
slightly modified the API to "int errdetail_relkind_not_supported(Oid
relid, Form_pg_class rd_rel);" to simplify things and increase the
usability of the function further. For instance, it can report the
specific error for the catalog tables as well. And, also added "int
errdetail_relkind_not_supported _v2(Oid relid, char relkind, char
relpersistence);" so that the callers not having Form_pg_class (there
are 3 callers exist) can pass the parameters directly.

PSA v10.

Regards,
Bharath Rupireddy.

On Sat, Nov 13, 2021, at 12:00 AM, Bharath Rupireddy wrote:
> On Sat, Nov 13, 2021 at 12:06 AM Euler Taveira <euler(at)eulerto(dot)com> wrote:
> > > Here's a rebased v8 patch. Please review it.
> >
> > This improves the user experience by increasing the granularity of error
> > reporting, and maps well with the precedent set in 81d5995b4. I'm marking this
> > Ready for Committer and will go ahead and apply this unless there are
> > objections.
> >
> > Shouldn't we modify errdetail_relkind_not_supported() to include relpersistence
> > as a 2nd parameter and move those messages to it? I experiment this idea with
> > the attached patch. The idea is to provide a unique function that reports
> > accurate detail messages.
>
> Thanks. It is a good idea to use errdetail_relkind_not_supported. I
> slightly modified the API to "int errdetail_relkind_not_supported(Oid
> relid, Form_pg_class rd_rel);" to simplify things and increase the
> usability of the function further. For instance, it can report the
> specific error for the catalog tables as well. And, also added "int
> errdetail_relkind_not_supported _v2(Oid relid, char relkind, char
> relpersistence);" so that the callers not having Form_pg_class (there
> are 3 callers exist) can pass the parameters directly.
Do we really need 2 functions? I don't think so. Besides that, relid is
redundant since this information is available in the Form_pg_class struct.

int errdetail_relkind_not_supported(Oid relid, Form_pg_class rd_rel);

My suggestion is to keep only the 3 parameter function:

int errdetail_relkind_not_supported(Oid relid, char relkind, char relpersistence);

Multiple functions that is just a wrapper for a central one is a good idea for
backward compatibility. That's not the case here.

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

On Sat, Nov 13, 2021 at 7:16 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:
> Thanks. It is a good idea to use errdetail_relkind_not_supported. I
> slightly modified the API to "int errdetail_relkind_not_supported(Oid
> relid, Form_pg_class rd_rel);" to simplify things and increase the
> usability of the function further. For instance, it can report the
> specific error for the catalog tables as well. And, also added "int
> errdetail_relkind_not_supported _v2(Oid relid, char relkind, char
> relpersistence);" so that the callers not having Form_pg_class (there
> are 3 callers exist) can pass the parameters directly.
>
> Do we really need 2 functions? I don't think so. Besides that, relid is
> redundant since this information is available in the Form_pg_class struct.

Yeah. The relid is available in Form_pg_class.

Firstly, I didn't quite like the function
errdetail_relkind_not_supported name to be too long here and adding to
it the 2 or 3 parameters, in many places we are crossing 80 char
limit. Above these, a function with one parameter is always better
than function with 3 parameters.

Having two functions isn't a big deal at all, I think we have many
functions like that in the core (although I'm not gonna spend time
finding all those functions, I'm sure there will be such functions).

I would still go with with 2 functions:

int errdetail_relkind_not_supported(Form_pg_class rd_rel);
int errdetail_relkind_not_supported_v2(Oid relid, char relkind, char
relpersistence);

> int errdetail_relkind_not_supported(Oid relid, Form_pg_class rd_rel);
>
> My suggestion is to keep only the 3 parameter function:
>
> int errdetail_relkind_not_supported(Oid relid, char relkind, char relpersistence);
>
> Multiple functions that is just a wrapper for a central one is a good idea for
> backward compatibility. That's not the case here.

Since we are modifying it on the master, I think it is okay to have 2
functions given the code simplification advantages we get with
errdetail_relkind_not_supported(Form_pg_class rd_rel).

I would even think further to rename "errdetail_relkind_not_supported"
and have the following, because we don't have to be always descriptive
in the function names. The errdetail would tell the function is going
to give some error detail.

int errdetail_relkind(Form_pg_class rd_rel);
int errdetail_relkind_v2(Oid relid, char relkind, char relpersistence);

or

int errdetail_rel(Form_pg_class rd_rel);
int errdetail_rel_v2(Oid relid, char relkind, char relpersistence);

I prefer the above among the three function names.

Thoughts?

Regards,
Bharath Rupireddy.

On Sat, Nov 13, 2021 at 7:57 PM Bharath Rupireddy <
bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Sat, Nov 13, 2021 at 7:16 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:
> > Thanks. It is a good idea to use errdetail_relkind_not_supported. I
> > slightly modified the API to "int errdetail_relkind_not_supported(Oid
> > relid, Form_pg_class rd_rel);" to simplify things and increase the
> > usability of the function further. For instance, it can report the
> > specific error for the catalog tables as well. And, also added "int
> > errdetail_relkind_not_supported _v2(Oid relid, char relkind, char
> > relpersistence);" so that the callers not having Form_pg_class (there
> > are 3 callers exist) can pass the parameters directly.
> >
> > Do we really need 2 functions? I don't think so. Besides that, relid is
> > redundant since this information is available in the Form_pg_class
struct.
>
> Yeah. The relid is available in Form_pg_class.
>
> Firstly, I didn't quite like the function
> errdetail_relkind_not_supported name to be too long here and adding to
> it the 2 or 3 parameters, in many places we are crossing 80 char
> limit. Above these, a function with one parameter is always better
> than function with 3 parameters.
>
> Having two functions isn't a big deal at all, I think we have many
> functions like that in the core (although I'm not gonna spend time
> finding all those functions, I'm sure there will be such functions).
>
> I would still go with with 2 functions:
>
> int errdetail_relkind_not_supported(Form_pg_class rd_rel);
> int errdetail_relkind_not_supported_v2(Oid relid, char relkind, char
> relpersistence);
>
> > int errdetail_relkind_not_supported(Oid relid, Form_pg_class rd_rel);
> >
> > My suggestion is to keep only the 3 parameter function:
> >
> > int errdetail_relkind_not_supported(Oid relid, char relkind, char
relpersistence);
> >
> > Multiple functions that is just a wrapper for a central one is a good
idea for
> > backward compatibility. That's not the case here.
>
> Since we are modifying it on the master, I think it is okay to have 2
> functions given the code simplification advantages we get with
> errdetail_relkind_not_supported(Form_pg_class rd_rel).
>
> I would even think further to rename "errdetail_relkind_not_supported"
> and have the following, because we don't have to be always descriptive
> in the function names. The errdetail would tell the function is going
> to give some error detail.
>
> int errdetail_relkind(Form_pg_class rd_rel);
> int errdetail_relkind_v2(Oid relid, char relkind, char relpersistence);
>
> or
>
> int errdetail_rel(Form_pg_class rd_rel);
> int errdetail_rel_v2(Oid relid, char relkind, char relpersistence);
>
> I prefer the above among the three function names.
>
> Thoughts?

PSA v11 patch with 2 APIs with much simpler parameters and small function
names:

int errdetail_rel(Form_pg_class rd_rel);
int errdetail_rel_v2(Oid relid, char relkind, char relpersistence);

Please review it.

Regards,
Bharath Rupireddy.

On 14.11.21 13:18, Bharath Rupireddy wrote:
> PSA v11 patch with 2 APIs with much simpler parameters and small
> function names:
>
> int errdetail_rel(Form_pg_class rd_rel);
> int errdetail_rel_v2(Oid relid, char relkind, char relpersistence);
>
> Please review it.

I think this is not an improvement. It loses the ability of the caller
the specify exactly why a relation is not acceptable. Before, a caller
could say, it's the wrong relkind, or it's the wrong persistence, or
whatever. Now, it just spits out some details about the relation, but
you can't control which. It could easily be wrong, too: AFAICT, this
will complain that a temporary table is not supported, but it could also
be that a table in general is not supported.

In my mind, this leads us back into the mess that we have before
errdetail_relkind_not_supported(): Very detailed error messages that
didn't actually hit the point.

I think a separate errdetail_relpersistence_not_supported() would be a
better solution (assuming there are enough callers to make it worth a
separate function).

> On 15 Nov 2021, at 09:15, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:

> I think this is not an improvement. It loses the ability of the caller the
> specify exactly why a relation is not acceptable.

Agreed.

> I think a separate errdetail_relpersistence_not_supported() would be a better
> solution (assuming there are enough callers to make it worth a separate
> function).

I still think that the v8 patch posted earlier is the better option, which
increase granularity of error reporting with a small code footprint.

--
Daniel Gustafsson https://vmware.com/

On Mon, Nov 15, 2021 at 2:14 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
> > On 15 Nov 2021, at 09:15, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
>
> > I think this is not an improvement. It loses the ability of the caller the
> > specify exactly why a relation is not acceptable.
>
>
> Agreed.

+1.

> > I think a separate errdetail_relpersistence_not_supported() would be a better
> > solution (assuming there are enough callers to make it worth a separate
> > function).
>
>
> I still think that the v8 patch posted earlier is the better option, which
> increase granularity of error reporting with a small code footprint.

Thanks. Attaching the v8 here again.

Regards,
Bharath Rupireddy.

On 15.11.21 10:38, Bharath Rupireddy wrote:
>> I still think that the v8 patch posted earlier is the better option, which
>> increase granularity of error reporting with a small code footprint.
> Thanks. Attaching the v8 here again.

I find the use of RelationUsesLocalBuffers() confusing in this patch.
It would be clearer to check relpersistence directly in both branches of
the if statement.

> On 15 Nov 2021, at 19:42, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
>
> On 15.11.21 10:38, Bharath Rupireddy wrote:
>>> I still think that the v8 patch posted earlier is the better option, which
>>> increase granularity of error reporting with a small code footprint.
>> Thanks. Attaching the v8 here again.
>
> I find the use of RelationUsesLocalBuffers() confusing in this patch. It would be clearer to check relpersistence directly in both branches of the if statement.

Admittedly it didn't bother me, but the more I think about it the more I agree
with Peter, so +1 on changing.

--
Daniel Gustafsson https://vmware.com/

On Tue, Nov 16, 2021 at 3:06 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
> > On 15 Nov 2021, at 19:42, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
> >
> > On 15.11.21 10:38, Bharath Rupireddy wrote:
> >>> I still think that the v8 patch posted earlier is the better option, which
> >>> increase granularity of error reporting with a small code footprint.
> >> Thanks. Attaching the v8 here again.
> >
> > I find the use of RelationUsesLocalBuffers() confusing in this patch. It would be clearer to check relpersistence directly in both branches of the if statement.
>
> Admittedly it didn't bother me, but the more I think about it the more I agree
> with Peter, so +1 on changing.

Done. PSA v9 patch.

Regards,
Bharath Rupireddy.

> On 16 Nov 2021, at 03:30, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:

> Done. PSA v9 patch.

Pushed with some tweaking of the commit message, thanks!

--
Daniel Gustafsson https://vmware.com/