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

On Wed, Mar 10, 2021 at 10:44 AM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.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.ladhe@enterprisedb.com> wrote:

On Wed, Mar 10, 2021 at 10:44 AM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.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@eulerto.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.rupireddyforpostgres@gmail.com> wrote:

On Wed, Mar 10, 2021 at 5:48 PM Euler Taveira <euler@eulerto.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.rupireddyforpostgres@gmail.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.rupireddyforpostgres@gmail.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@eulerto.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.rupireddyforpostgres@gmail.com> wrote:

On Mon, Apr 5, 2021 at 6:41 PM Euler Taveira <euler@eulerto.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@gmail.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.rupireddyforpostgres@gmail.com> wrote:

On Wed, May 26, 2021 at 7:38 PM vignesh C <vignesh21@gmail.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@gmail.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.rupireddyforpostgres@gmail.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.rupireddyforpostgres@gmail.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@eisentraut.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.rupireddyforpostgres@gmail.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@yesql.se> wrote:

On 26 Jul 2021, at 09:33, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.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.rupireddyforpostgres@gmail.com> wrote:

On Wed, Nov 3, 2021 at 6:21 PM Daniel Gustafsson <daniel@yesql.se> wrote:

On 26 Jul 2021, at 09:33, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.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.rupireddyforpostgres@gmail.com> wrote:

On Wed, Nov 3, 2021 at 6:21 PM Daniel Gustafsson <daniel@yesql.se> wrote:

On 26 Jul 2021, at 09:33, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.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@eulerto.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.