From: | vignesh C <vignesh21(at)gmail(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Peter Smith <smithpb2250(at)gmail(dot)com>, "tanghy(dot)fnst(at)fujitsu(dot)com" <tanghy(dot)fnst(at)fujitsu(dot)com>, Greg Nancarrow <gregn4422(at)gmail(dot)com>, Ajin Cherian <itsajin(at)gmail(dot)com>, Rahila Syed <rahilasyed90(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
Subject: | Re: Added schema level support for publication. |
Date: | 2021-09-21 18:02:09 |
Message-ID: | CALDaNm3aFtXpkD4m28-ENG9F4faBEVdGNUrEhgKV0pHr2S_C2g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Sep 20, 2021 at 3:56 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Sep 17, 2021 at 5:39 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Wed, Sep 15, 2021 at 12:30 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
v30-0001-Made-the-existing-relation-cache-invalidation-an.patch | text/x-patch | 5.9 KB |
v30-0002-Added-schema-level-support-for-publication.patch | text/x-patch | 77.5 KB |
v30-0003-Client-side-changes-to-support-FOR-ALL-TABLES-IN.patch | text/x-patch | 21.6 KB |
v30-0004-Tests-for-FOR-ALL-TABLES-IN-SCHEMA-publication.patch | text/x-patch | 52.2 KB |
v30-0005-Documentation-for-FOR-ALL-TABLES-IN-SCHEMA-publi.patch | text/x-patch | 15.4 KB |
v30-0006-Implemented-pg_publication_objects-view.patch | text/x-patch | 6.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | vignesh C | 2021-09-21 18:04:40 | Re: Added schema level support for publication. |
Previous Message | Tom Lane | 2021-09-21 17:55:24 | Re: Re: [PATCH] regexp_positions ( string text, pattern text, flags text ) → setof int4range[] |