Re: Added schema level support for publication.

On Fri, Aug 27, 2021 at 6:09 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Fri, Aug 27, 2021 at 4:57 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Fri, Aug 27, 2021 at 11:43 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Aug 25, 2021 at 3:07 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > I have implemented this in the 0003 patch, I have kept it separate to
> > > reduce the testing effort and also it will be easier if someone
> > > disagrees with the syntax. I will merge it to the main patch later
> > > based on the feedback. Attached v22 patch has the changes for the
> > > same.
> > >
> >
> > Few comments on v22-0001-Added-schema-level-support-for-publication:
> > ========================================================
> > 1. Why in publication_add_schema(), you are registering invalidation
> > for all schema relations? It seems this is to allow rebuilding the
> > publication info for decoding sessions. But that is not required as
> > you are registering rel_sync_cache_publication_cb for
> > publication_schema relation. In rel_sync_cache_publication_cb, we are
> > marking replicate_valid as false for each entry which will allow
> > publication info to be rebuilt in get_rel_sync_entry.
> >
> > I see that it is done for a single relation in the current code in
> > function publication_add_relation but I guess that is also not
> > required. You can once test this. If you still think it is required,
> > can you please explain me why and then we can accordingly add some
> > comments in the patch.
>
> I felt this is required for handling the following concurrency scenario:
> create schema sch1;
> create table sch1.t1(c1 int);
> insert into sch1.t1 values(10);
> update sch1.t1 set c1 = 11;
> # update will be successful and relation cache will update publication
> actions based on the current state i.e no publication
> create publication pub1 for all tables in schema sch1;
> # After the above publication is created the relations present in this
> schema should be invalidated so that the next update should fail.
>

What do you mean by update should fail? I think all the relations in
RelationSyncCache via rel_sync_cache_publication_cb because you have
updated pg_publication_schema and that should register syscache
invalidation.

> If
> the relations are not invalidated the updates will be successful based
> on previous publication actions.
> update sch1.t1 set c1 = 11;
>

I think even without special relcache invalidations the relations
should be invalidated because of syscache invalidation as mentioned in
the previous point. Am I missing something here?

>
> > 5. Don't we need to expose a view for publication schemas similar to
> > pg_publication_tables?
>
> pg_publication_tables is a common view for both "FOR TABLE", "FOR ALL
> TABLES" and "FOR ALL TABLES IN SCHEMA", this view will internally
> access pg_publication_rel and pg_publication_schema to get the
> corresponding tables. I felt we don't need a separate view for
> publication schemas. Thoughts?
>

Don't you think some users might want to know all the schema names for
a publication? I am not completely sure on this point but I think it
is good to have information for users. It might be also useful to have
pg_publication_objects where we can display object types (like table,
schema, sequence, etc) and then object names. If you are not convinced
then we can wait and see what others think about this.

--
With Regards,
Amit Kapila.