Re: Added schema level support for publication.

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
To: vignesh C <vignesh21(at)gmail(dot)com>
Cc: "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>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(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-08-05 10:23:49
Message-ID: CAA4eK1Jjjp-JZOgT=kmYPjKSnUJyCPZxxHhiirtiMAObvAMi-w@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Aug 4, 2021 at 4:10 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Aug 3, 2021 at 8:38 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > Thanks for reporting this, this is fixed in the v18 patch attached.
> >
>
> I have started looking into this patch and below are some initial comments.
>

Few more comments:
===================
1. Do we need the previous column 'puballtables' after adding pubtype
or pubkind in pg_publication?

2.
@@ -224,6 +279,20 @@ CreatePublication(ParseState *pstate,
CreatePublicationStmt *stmt)
..
+ nspcrel = table_open(NamespaceRelationId, ShareUpdateExclusiveLock);
+ PublicationAddSchemas(puboid, schemaoidlist, true, NULL);
+ table_close(nspcrel, ShareUpdateExclusiveLock);

What is the reason for opening and taking a lock on
NamespaceRelationId? Do you want to avoid dropping the corresponding
schema during this duration? If so, that is not sufficient because
what if somebody drops it just before you took lock on
NamespaceRelationId. I think you need to use LockDatabaseObject to
avoid dropping the schema and note that it should be unlocked only at
the end of the transaction similar to what we do for tables. I guess
you need to add this locking inside the function
PublicationAddSchemas. Also, it would be good if you can add few
comments in this part of the code to explain the reason for locking.

3. The function PublicationAddSchemas is called from multiple places
in the patch but the locking protection is not there at all places. I
think if you add locking as suggested in the previous point then it
should be okay. I think you need similar locking for
PublicationDropSchemas.

4.
@@ -421,16 +537,84 @@ AlterPublicationTables(AlterPublicationStmt
*stmt, Relation rel,
PublicationAddTables(pubid, rels, true, stmt);

CloseTableList(delrels);
+ if (pubform->pubtype == PUBTYPE_EMPTY)
+ UpdatePublicationTypeTupleValue(rel, tup,
+ Anum_pg_publication_pubtype,
+ PUBTYPE_TABLE);
}

At the above and all other similar places, the patch first updates the
pg_publication after performing the rel/schema operation. Isn't it
better to first update pg_publication to remain in sync with how
CreatePublication works? I am not able to see any issue with the way
you have it in the patch but it is better to keep the code consistent
across various similar functions to avoid confusion in the future.

--
With Regards,
Amit Kapila.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Platon Pronko 2021-08-05 10:36:45 very long record lines in expanded psql output
Previous Message Paul Guo 2021-08-05 10:20:44 Re: standby recovery fails (tablespace related) (tentative patch and discussion)