Re: Added schema level support for publication.

On Wed, Jun 23, 2021 at 2:10 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
>
> On Tue, Jun 22, 2021 at 2:15 PM vignesh C <vignesh21(at)gmail(dot)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.
>

Modified.

> (2) src/backend/catalog/objectaddress.c
> getObjectIdentityParts(), case OCLASS_PUBLICATION_SCHEMA
>
> Shouldn't pubname/nspname be pfree()d, if objargs/objname are NULL?
>

Modified

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

Modified

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

Modified

> (4) src/backend/commands/publicationcmds.c
>
> Missing function header for function "UpdatePublicationTypeTupleValue".
>

Included function header.

> (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
> + */
>

Modified it to:
/*
* 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;
>

Modified.

> (7) src/bin/psql/describe.c
>
> Missing function header for function "addFooterToPublicationDesc".
>

Included it.

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

Modified

> (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":
>

Modified.

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

Modified.

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

Modified.

Thanks for reviewing and providing the comments, Attached v8 patches
have the fixes for the comments.

Regards,
Vignesh