Re: Added schema level support for publication.

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. If
the relations are not invalidated the updates will be successful based
on previous publication actions.
update sch1.t1 set c1 = 11;
I will add comments to mention the above details. Thoughts?

> Peter E., Sawada-San, can you please let me know if I am missing
> something in this regard? In the code, I see a comment "/* Invalidate
> relcache so that publication info is rebuilt. */" in function
> publication_add_relation() but I don't understand why it is required
> as per my explanation above?
>
> 2.
> + * Publication object type
> + */
> +typedef enum PublicationObjSpecType
> +{
> + PUBLICATIONOBJ_TABLE, /* Table type */
> + PUBLICATIONOBJ_SCHEMA, /* Schema type */
> + PUBLICATIONOBJ_SEQUENCE, /* Sequence type */
>
> Why add anything related to the sequence in this patch?

I will handle this in my next version.

> 3.
> +get_object_address_publication_schema(List *object, bool missing_ok)
> +{
> + ObjectAddress address;
> + char *pubname;
> + Publication *pub;
> + char *schemaname;
> + Oid schemaoid;
> +
> + ObjectAddressSet(address, PublicationSchemaRelationId, InvalidOid);
> +
> + /* Fetch schema name and publication name from input list */
> + schemaname = strVal(linitial(object));
> + pubname = strVal(lsecond(object));
> +
> + schemaoid = get_namespace_oid(schemaname, false);
> +
> + /* Now look up the pg_publication tuple */
> + pub = GetPublicationByName(pubname, missing_ok);
> + if (!pub)
> + return address;
>
> Why the schema name handling is different from publication name? Why
> can't we pass missing_ok for schema api and handle it similar
> publication api?

I will handle this in my next version.

> 4. In getPublicationSchemaInfo(), why the missing_ok flag is not used
> in get_publication_name() whereas it is used for all other syscache
> searches in that function?

I will handle this in my next version.

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

> 6.
> publication_add_schema()
> {
> ..
> + /* Can't be system namespace */
> + if (IsCatalogNamespace(schemaoid) || IsToastNamespace(schemaoid))
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("\"%s\" is a system schema",
> + get_namespace_name(schemaoid)),
> + errdetail("System schemas cannot be added to publications.")));
> +
> + /* Can't be temporary namespace */
> + if (isAnyTempNamespace(schemaoid))
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("\"%s\" is a temporary schema",
> + get_namespace_name(schemaoid)),
> + errdetail("Temporary schemas cannot be added to publications.")));
> ..
> }
>
> Can we change the first detail message as: "This operation is not
> supported for system schemas." and the second detail message as:
> "Temporary schemas cannot be replicated."? This is to make these
> messages similar to corresponding messages for relations in function
> check_publication_add_relation(). Can we move these checks to a
> separate function?

I will handle this in my next version.

Regards,
Vignesh