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

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/