RE: Added schema level support for publication.

From: vignesh C <vignesh21@gmail.com>
Sent: Thursday, July 22, 2021 1:38 AM
To: Rahila Syed <rahilasyed90@gmail.com>
Cc: Greg Nancarrow <gregn4422@gmail.com>; Tang, Haiying/唐 海英 <tanghy.fnst@fujitsu.com>; Ajin Cherian <itsajin@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Subject: Re: Added schema level support for publication.

On Wed, Jul 21, 2021 at 3:14 PM Rahila Syed <rahilasyed90@gmail.com<mailto:rahilasyed90@gmail.com>> wrote:
>
>
>
> On Mon, Jul 19, 2021 at 2:41 PM Greg Nancarrow <gregn4422@gmail.com<mailto:gregn4422@gmail.com>> wrote:
>>
>> On Fri, Jul 16, 2021 at 8:13 PM vignesh C <vignesh21@gmail.com<mailto:vignesh21@gmail.com>> wrote:
>> >
>> > Modified.
>> >
>> > Thanks for the comments, these issues are fixed as part of the v12 patch posted at [1].
>> > [1]  - https://www.postgresql.org/message-id/CALDaNm3V9ny5dJM8nofLGJ3zDuDG0gS2dX%2BAhDph--U5y%2B4VbQ%40mail.gmail.com
>> >
>>
>> There seems to be a problem with ALTER PUBLICATION ... SET TABLE ...
>> After that command, it still regards it as an empty (e) publication,
>> so I can then ALTER PUBLICATION ... ADD SCHEMA ...
>>
>
> One issue here is that the code to update publication type is missing
> in AlterPublicationTables for SET TABLE command.

Modified.

> More broadly, I am not clear about the behaviour of the patch when a
> publication is created to publish only certain tables, and is later altered to publish
> a whole schema. I think such behaviour is legitimate. However,
> AFAIU as per current code we can't update the publication type
> from PUBTYPE_TABLE to PUBTYPE_SCHEMA.

I initially thought this might not be required for users, I have not made any change for this, I will try to get a few more people's opinion on this and then fix it if required.

> I have some review comments as follows:
> 1.
> In ConvertSchemaSpecListToOidList(List *schemas) function:
>  +     search_path = fetch_search_path(false);
>  +                               nspname = get_namespace_name(linitial_oid(search_path));
>  +                               if (nspname == NULL)    /* recently-deleted namespace? */
>  +                                       ereport(ERROR,
>  +                                                       errcode(ERRCODE_UNDEFINED_SCHEMA),
>  +                                                       errmsg("no schema has been selected"));
>  +
>  +                               schemoid = get_namespace_oid(nspname, false);
>  +                               break;
>
> The call get_namespace_oid() is perhaps not needed as fetch_search_path already fetches oids and simply
> doing Schema oid = liinital_oid(search_path)); should be enough.

Modified

> 2. In the same function should there be an if else condition block instead of a switch case as
> there are only two cases.

Modified.

Thanks for the comments, these comments are fixed in the v13 patch posted at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm0%3DMaXyAok5iq_-DeWUd81vpdF47-MZbbrsd%2BzB2P6WwA%40mail.gmail.com

Regards,
Vignesh