Re: Added schema level support for publication.

On Tue, Jun 22, 2021 at 2:15 PM vignesh C <vignesh21@gmail.com> wrote:
>
>
> Updated patch has the fix for this, this also includes the fixes for
> the other comments you had given.
> I have removed the skip table patches to keep the focus on the main
> patch, once this patch gets into committable shape, I will focus on
> the skip table patch.
>

I have the following initial comments on the v7 patches:

v7-0001

(1) The patch comment is pretty rough and needs work.

I suggest something like the following:

This patch adds schema-level support for publication.
A new schema option allows one or more schemas to be specified, whose tables
are selected by the publisher for sending the data to the subscriber.

pg_publication maintains information about the publication. Previously, the
"puballtables" bool column was used to indicate if the publication was the
"FOR ALL TABLES" type (if true) or the "FOR TABLE" type (if false). With the
introduction of the "FOR SCHEMA" publication type, it is not easy to determine
the publication type. Therefore, a new column "pubtype" has been added to the
pg_publication relation to indicate the publication type.
There was the possibility of avoiding addition of this new column, but that
would require checking puballtables of pg_publication and checking
pg_publication_rel for table type publication and then checking
pg_publication_schema for schema type publication. Instead, I preferred to add
the "pubtype" column, which makes things easier, and also will help support
new options in the future.
A new system table "pg_publication_schema" has been added, to maintain the
schemas that the user wants to publish through the publication. The
schema/publication/publication_schema dependency was created to handle the
corresponding renaming/removal of schemas to the publication/publication_schema
when the schema is renamed/dropped. The Decoder identifies if the relation is
part of the publication and replicates it to the subscriber. Changes were made
to pg_dump to handle pubtype updation in the pg_publication table when the
database is upgraded.

Prototypes present in pg_publication.h have been moved to publicationcmds.h so
that minimal data structures are exported to pg_dump and psql clients, as the
rest of the information need not be exported.

CATALOG_VERSION_NO needs to be updated while committing, as this feature
involves a catalog change.

TODO: version checks for psql/pg_dump need to be changed from 140000 to 150000
once the ongoing release is completed.


(2) src/backend/catalog/objectaddress.c
getObjectIdentityParts(), case OCLASS_PUBLICATION_SCHEMA

Shouldn't pubname/nspname be pfree()d, if objargs/objname are NULL?


(3) src/backend/catalog/pg_publication.c

(i)
GetAllTablesPublications


BEFORE:
+ * Gets list of relations published.
AFTER:
+ * Gets the list of relations published.

There are several other cases of newly-added "Gets list of ..." comments.


(ii)
GetAllTablesPublicationRelations

BEFORE:
+ * Gets list of all relation published by FOR SCHEMA publication(s).
AFTER:
+ * Gets the list of all relations published by FOR SCHEMA publication(s).


(4) src/backend/commands/publicationcmds.c

Missing function header for function "UpdatePublicationTypeTupleValue".


(5) src/backend/parser/gram.y

Violation of function header comment format:

BEFORE:
+/* makeSchemaSpec
+ * Create a SchemaSpec with the given type
+ */

AFTER:
+/*
+ * makeSchemaSpec
+ * Create a SchemaSpec with the given type
+ */


(6) src/bin/pg_dump/pg_dump.c

The following code is within a loop that processes schemas.
I think that (in the comment) "Clean up and return" should instead say
"Clean up and process the next schema"
Also, should say "Schema is not a member".

+ /*
+ * Schema is not member of any publications. Clean up and return.
+ */
+ PQclear(res);
+ continue;


(7) src/bin/psql/describe.c

Missing function header for function "addFooterToPublicationDesc".


v7-0002

(1) doc/src/sgml/catalogs.sgml
Typo

BEFORE:
+       respctively. The publication type cannot be changed in other cases.
AFTER:
+       respectively. The publication type cannot be changed in other cases.


(2) doc/src/sgml/ref/create_publication.sgml

BEFORE:
+   Create a publication that publishes all changes for all the tables
present in
+production schema:
AFTER:
+   Create a publication that publishes all changes for all the tables
present in
+the schema "production":


BEFORE:
+   Create a publication that publishes all changes for all the tables
present in
+marketing and sales schemas:
AFTER:
+   Create a publication that publishes all changes for all the tables
present in
+the schemas "marketing" and "sales":


(3) src/test/regress/expected/publication.out

BEFORE:
+-- Drop schema that is not preset in the publication
AFTER:
+-- Drop schema that is not present in the publication

BEFORE:
+--- Check create publication on a object which is not schema
AFTER:
+--- Check create publication on an object which is not schema


(4) src/test/regress/sql/publication.sql

BEFORE:
+-- Drop schema that is not preset in the publication
AFTER:
+-- Drop schema that is not present in the publication


Regards,
Greg Nancarrow
Fujitsu Australia