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

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