RE: Added schema level support for publication.

From: "tanghy(dot)fnst(at)fujitsu(dot)com" <tanghy(dot)fnst(at)fujitsu(dot)com>
To: vignesh C <vignesh21(at)gmail(dot)com>
Cc: Ajin Cherian <itsajin(at)gmail(dot)com>, Rahila Syed <rahilasyed90(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>
Subject: RE: Added schema level support for publication.
Date: 2021-07-13 06:36:09
Message-ID: OS0PR01MB61137F0B8498E9241F2D960EFB149@OS0PR01MB6113.jpnprd01.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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\"",

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.

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.

Regards
Tang

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message gkokolatos 2021-07-13 06:36:59 Re: Introduce pg_receivewal gzip compression tests
Previous Message Julien Rouhaud 2021-07-13 06:10:23 Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?