Обсуждение: Re: Logical Replication - improve error message while adding tables to the publication in check_publication_add_relation

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

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

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.

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.

On Sat, Nov 13, 2021 at 7:16 PM Euler Taveira <euler@eulerto.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 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.eisentraut@enterprisedb.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@yesql.se> wrote:
>
> > On 15 Nov 2021, at 09:15, Peter Eisentraut <peter.eisentraut@enterprisedb.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.eisentraut@enterprisedb.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
directlyin 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@yesql.se> wrote:
>
> > On 15 Nov 2021, at 19:42, Peter Eisentraut <peter.eisentraut@enterprisedb.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
directlyin 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.rupireddyforpostgres@gmail.com> wrote:

> Done. PSA v9 patch.

Pushed with some tweaking of the commit message, thanks!

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