Re: Added schema level support for publication. - Mailing list pgsql-hackers

On Wed, Sep 15, 2021 at 12:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Sep 14, 2021 at 2:08 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > I have handled this in the patch attached.
> >
>
> Few comments:
> =============
> 1.
> + * CREATE PUBLICATION FOR pub_obj [, pub_obj2] [WITH options]
> + *
> + * pub_obj is one of:
> + *
> + * TABLE table [, table2]
> ..
> ..
> - * ALTER PUBLICATION name ADD TABLE table [, table2]
> + * ALTER PUBLICATION name ADD pub_obj [, pub_obj ...]
>   *
> - * ALTER PUBLICATION name DROP TABLE table [, table2]
> + * ALTER PUBLICATION name DROP pub_obj [, pub_obj ...]
>   *
> - * ALTER PUBLICATION name SET TABLE table [, table2]
> + * ALTER PUBLICATION name SET pub_obj [, pub_obj ...]
>
> In all the above places, the object names mentioned in square brackets
> are not consistent. I suggest using [, ...] everywhere as that is what
> we are using in docs as well.

Modified

> 2.
> +/*
> + * Check if the relation schema is member of the schema list.
> + */
> +static void
> +RelSchemaIsMemberOfSchemaList(List *rels, List *schemaidlist, bool schemacheck)
>
> Can we change the above comment as: "Check if any of the given
> relation's schema is a member of the given schema list."?

Modified

> 3.
> + errmsg("cannot add relation \"%s.%s\" to publication",
> + get_namespace_name(relSchemaId),
> + RelationGetRelationName(rel)),
> + errdetail("Table's schema \"%s\" is already part of the publication.",
> + get_namespace_name(relSchemaId)));
>
> This and other parts of error messages in
> +RelSchemaIsMemberOfSchemaList are not aligned. I think you can now
> run pgindent on your patches that will solve the indentation issues in
> the patch.

Changed by running pgindent.

> 4.
> AlterPublicationSchemas()
> {
> ..
> + /*
> + * If the table option was not specified remove the existing tables
> + * from the publication.
> + */
> + if (!tables)
> + {
> + rels = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT);
> + PublicationDropTables(pubform->oid, rels, false, true);
> + }
> +
> + /* Identify which schemas should be dropped */
> + delschemas = list_difference_oid(oldschemaids, schemaidlist);
> +
> + /* And drop them */
> + PublicationDropSchemas(pubform->oid, delschemas, true);
>
> Here, you have neither locked tables to be dropped nor schemas. I
> think both need to be locked as we do for tables in similar code in
> AlterPublicationTables(). Can you please test via debugger what
> happens if we try to drop without taking lock here and concurrently
> try to drop the actual object? It should give some error. If we decide
> to lock here then we should be able to pass the list of relations to
> PublicationDropTables() instead of Oids which would then obviate the
> need for any change to that function.
>
> Similarly don't we need to lock schemas before dropping them in
> AlterPublicationTables()?

we will get the following error, if concurrently dropped from another
session during debugging:
postgres=# alter publication pub1 set all tables in schema sch2;
ERROR:  cache lookup failed for publication table 16418
Modified to add locking

> 5.
> +/*
> + * Find the ObjectAddress for a publication tables in schema.  The first
> + * element of the object parameter is the schema name, the second is the
> + * publication name.
> + */
> +static ObjectAddress
> +get_object_address_publication_schema(List *object, bool missing_ok)
>
> The first part of the above comment is not clear. Can we write it as:
> "Find the ObjectAddress for a publication schema. .."?

Modified

> 6.
> +List *
> +GetSchemaPublicationRelations(Oid schemaid, PublicationPartOpt pub_partopt)
> +{
> + Relation classRel;
> + ScanKeyData key[3];
> + TableScanDesc scan;
> + HeapTuple tuple;
> + List    *result = NIL;
> + int keycount = 0;
> +
> + Assert(schemaid != InvalidOid);
>
> Isn't it better to use OidIsValid() in the above assert?

Modified

> 7.
> @@ -974,6 +974,7 @@ EventTriggerSupportsObjectType(ObjectType obtype)
>   case OBJECT_PROCEDURE:
>   case OBJECT_PUBLICATION:
>   case OBJECT_PUBLICATION_REL:
> + case OBJECT_PUBLICATION_REL_IN_NAMESPACE:
>   case OBJECT_ROUTINE:
>   case OBJECT_RULE:
>   case OBJECT_SCHEMA:
> @@ -1050,6 +1051,7 @@ EventTriggerSupportsObjectClass(ObjectClass objclass)
>   case OCLASS_EXTENSION:
>   case OCLASS_POLICY:
>   case OCLASS_PUBLICATION:
> + case OCLASS_PUBLICATION_NAMESPACE:
>   case OCLASS_PUBLICATION_REL:
>   case OCLASS_SUBSCRIPTION:
>   case OCLASS_TRANSFORM:
> @@ -2127,6 +2129,7 @@ stringify_grant_objtype(ObjectType objtype)
>   case OBJECT_POLICY:
>   case OBJECT_PUBLICATION:
>   case OBJECT_PUBLICATION_REL:
> + case OBJECT_PUBLICATION_REL_IN_NAMESPACE:
>   case OBJECT_ROLE:
>   case OBJECT_RULE:
>   case OBJECT_STATISTIC_EXT:
> @@ -2209,6 +2212,7 @@ stringify_adefprivs_objtype(ObjectType objtype)
>   case OBJECT_POLICY:
>   case OBJECT_PUBLICATION:
>   case OBJECT_PUBLICATION_REL:
> + case OBJECT_PUBLICATION_REL_IN_NAMESPACE:
>
> What is the reason for using different names for object_class and
> object_type? Normally, we use the same. Is it making things clear in
> any place?

I thought it might be easier to review, I have changed it to the
standard way of naming object_class and object_type. Renamed it to
OBJECT_PUBLICATION_NAMESPACE.

> 8.
> + if (stmt->action == DEFELEM_ADD)
> + {
> + List *pubschemas = GetPublicationSchemas(pubid);
> +
> + /* check if the relation is member of the schema list specified */
> + RelSchemaIsMemberOfSchemaList(rels, schemaidlist, false);
> +
> + /*
> + * Check if the relation is member of the existing schema in the
> + * publication.
> + */
> + RelSchemaIsMemberOfSchemaList(rels, pubschemas, false);
>
> Isn't it better to concat the list of schemas and then check the
> membership of relations once?

Modified.

Attached v29 patch has the fixes for the same.

Regards,
Vignesh