Re: Added schema level support for publication.

On Mon, Sep 20, 2021 at 3:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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?

Modified.

> 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

Modified

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

Modified

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

Modified

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

I have changed it to "(Node *)" without space in *.y, I did not change
in *.c as I noticed it is used with space like "(Node *) " in other
places of *.c files.

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

Modified

 And
> then remove the inheritance related comment from code below:
>
> + /* inheritance query, implicitly */
> + $$ = makeNode(PublicationObjSpec);
> + $$->object = $1;

Modified.

Attached v30 patch has the fixes for the same.

Regards,
Vignesh