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

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