Re: Restrict publishing of partitioned table with a foreign table as partition

30.01.2025 19:02, Shlok Kyal пишет:
> On Wed, 29 Jan 2025 at 19:21, Sergey Tatarintsev
> <s(dot)tatarintsev(at)postgrespro(dot)ru> wrote:
>>
>> 29.01.2025 12:16, Shlok Kyal пишет:
>>> Hi,
>>>
>>> As part of a discussion in [1], I am starting this thread to address
>>> the issue reported for foreign tables.
>>>
>>> Logical replication of foreign tables is not supported, and we throw
>>> an error in this case. But when we create a publication on a
>>> partitioned table that has a foreign table as a partition, the initial
>>> sync of such a table is successful. We should also throw an error in
>>> such cases.
>>> With this patch, we will throw an error when we try to create a
>>> publication on (or add to an existing publication) a partitioned table
>>> with a foreign table as its partition or attach such a table to
>>> existing published tables.
>>>
>>> [1] : https://www.postgresql.org/message-id/CAA4eK1Lhh4SgiYQLNiWSNKGdVSzbd53%3Dsr2tQCKooEphDkUtgw%40mail.gmail.com
>>>
>>> Thanks and Regards,
>>> Shlok Kyal
>> Hi!
>>
>> Thanks for patch.
>>
>> I reviewed it and made some changes.
>>
>> 1. we should check foreign tables (not partitioned)
>> 2. added checking for foreign table creation
>> 3. some little corrections
>>
>> See attach
>>
> Hi Sergey,
>
> I have added most of the changes in v2-0002 with small changes except
> one change.
>
> @@ -1428,6 +1427,12 @@ check_foreign_tables_in_schema(Oid schemaid)
> errdetail("foreign table \"%s\" is a partition of
> partitioned table \"%s\"",
> get_rel_name(foreign_tbl_relid), parent_name)));
> }
> + else if (relForm->relkind == RELKIND_FOREIGN_TABLE)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("cannot add relation \"%s\" to publication",
> + get_rel_name(relForm->oid)),
> + errdetail_relkind_not_supported(RELKIND_FOREIGN_TABLE)));
> }
>
> We should only throw error when foreign table is part of a partition
> table in case of 'FOR TABLES IN SCHEMA' . We should not throw an error
> otherwise because in case of 'FOR TABLES IN SCHEMA' foreign tables are
> not published by default.
>
> I have added the changes in v3-0001.
>
> Thanks and Regards,
> Shlok Kyal

Hello!

Ok, but maybe it will be correct to raise an WARNING (or at least LOG)
that some tables was skipped during publication. What do you think?

And I think we need check tables which was really published in case of
'FOR ALL TABLES' and 'FOR TABLES IN SCHEMA' in our tests. See attach.

Also it looks strange - we raise an ERROR for partitioned tables
containing foreign partitions , but just skip foreign tables as itself.
I think there should be the same behavior in both cases - raise an ERROR
or skip.