Re: Added schema level support for publication.

On Mon, Jul 19, 2021 at 2:41 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:

> On Fri, Jul 16, 2021 at 8:13 PM vignesh C <vignesh21(at)gmail(dot)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.

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 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.

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

Thank you,
Rahila Syed