Re: Added schema level support for publication.

On Tue, Jul 13, 2021 at 12:06 PM tanghy(dot)fnst(at)fujitsu(dot)com
<tanghy(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Monday, July 12, 2021 5:36 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > Thanks for reporting this issue, this issue is fixed in the v10 patch
> > attached at [1].
> > [1] - https://www.postgresql.org/message-id/CALDaNm2%2BtR%2B8R-
> > sD1CSyMbZcZbkintZE-avefjsp7LCkm6HMmw%40mail.gmail.com
>
> Thanks for fixing it.
>
> By applying your V10 patch, I saw three problems, please have a look.
>
> 1. An issue about pg_dump.
> When public schema was published, the publication was created in the output file, but public schema was not added to it. (Other schemas could be added as expected.)
>
> I looked into it and found that selectDumpableNamespace function marks DUMP_COMPONENT_DEFINITION as needless when the schema is public, leading to schema public is ignored in getPublicationSchemas. So we'd better check whether schemas should be dumped in another way.
>
> I tried to fix it with the following change, please have a look. (Maybe we also need to add some comments for it.)
>
> diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
> index f6b4f12648..a327d2568b 100644
> --- a/src/bin/pg_dump/pg_dump.c
> +++ b/src/bin/pg_dump/pg_dump.c
> @@ -4206,7 +4206,8 @@ getPublicationSchemas(Archive *fout, NamespaceInfo nspinfo[], int numSchemas)
> * Ignore publication membership of schemas whose definitions are not
> * to be dumped.
> */
> - if (!(nsinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
> + if (!((nsinfo->dobj.dump & DUMP_COMPONENT_DEFINITION)
> + || (strcmp(nsinfo->dobj.name, "public") == 0 && nsinfo->dobj.dump != DUMP_COMPONENT_NONE)))
> continue;
>
> pg_log_info("reading publication membership for schema \"%s\"",

I felt it is intentionally done like that as the pubic schema is
created by default, hence it is not required to dump else we will get
errors while restoring. Thougths?

> 2. improper behavior for system schema
> I found I could create publication for system schema, such as pg_catalog. I think it's better to report an error message here, just like creating publication for system table is unallowed.

Modified.

> 3. fix for dumpPublicationSchema
> Should we add a declaration for it and add const decorations to the it's second parameter? Like other similar functions.

Modified to include const, declaration is not required as the function
definition is before function call, so not making this change.

Thanks for your comments, the attached v11 patch fixes the issues.

Regards,
Vignesh