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

On Fri, Sep 17, 2021 at 5:39 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, Sep 15, 2021 at 12:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
>
> > 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
>

But you haven't followed my other suggestion related to
PublicationDropTables(). I don't think after doing this, you need to
pass 'true' as the last parameter to PublicationDropTables. In fact,
you can remove that parameter altogether or in other words, we don't
need any change in PublicationDropTables for this patch. Is there a
reason why we shouldn't make this change?

Few other comments:
===================
1. The ordering of lock acquisition for schema and relation in
AlterPublicationSchemas() and AlterPublicationTables() is opposite
which would generally lead to deadlock but it won't here because we
acquire share lock on the schema. But, I think it may still be better
to keep the locking order the same and it might help us to keep schema
and relation code separate

2. One more thing, I think one can concurrently add-relation for a
particular schema and that particular schema. To protect that
AlterPublication should acquire an exclusive lock similar to how we do
in AlterSubscription.

3.
+ /*
+ * If the table option was not specified remove the existing tables
+ * from the publication.
+ */
+ if (!relsSpecified)
+ {
+ List    *relations = NIL;
+ List    *tables = NIL;
+
+ rels = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT);
+ tables = RelationOidsToRangevars(rels);
+ relations = OpenTableList(tables);

One problem with using OpenTableList here is that it might try to lock
inherited children twice. Also, you don't need to first convert to
rangevar for locking relations, you can directly use table_open here.

4.
+ | extended_relation_expr
+ {
+ $$ = makeNode(PublicationObjSpec);
+ $$->object = (Node *)$1;
+ }
+ | CURRENT_SCHEMA
+ {
+ $$ = makeNode(PublicationObjSpec);
+ $$->object = (Node *)makeString("CURRENT_SCHEMA");
+ }
  ;

-publication_for_tables:
- FOR TABLE publication_table_list
+/* This can be either a schema or relation name. */
+pubobj_name:
+ ColId
  {
- $$ = (Node *) $3;
+ $$ = (Node *) makeString($1);
  }
- | FOR ALL TABLES
+ | ColId indirection
  {
- $$ = (Node *) makeInteger(true);
+ $$ = (Node *) makeRangeVarFromQualifiedName($1, $2, @1, yyscanner);
  }

In some places, you have given space after (Node *) and at other
places, there is no space. Isn't it better to be consistent?

5.
+/* This can be either a schema or relation name. */
+pubobj_name:

Here, we can modify the comment as "This can be either a schema or
relation name. For relations, the inheritance will be implicit." And
then remove the inheritance related comment from code below:

+ /* inheritance query, implicitly */
+ $$ = makeNode(PublicationObjSpec);
+ $$->object = $1;

-- 
With Regards,
Amit Kapila.