Re: Added schema level support for publication.

On Thu, Aug 12, 2021 at 5:54 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Aug 6, 2021 at 5:33 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for the comments, the attached v19 patch has the fixes for the comments.
>
> Thank you for updating the patch!
>
> Here are some comments on v19 patch:
>
> +                case OCLASS_PUBLICATION_SCHEMA:
> +                        RemovePublicationSchemaById(object->objectId);
> +                        break;
>
> +void
> +RemovePublicationSchemaById(Oid psoid)
> +{
> +        Relation       rel;
> +        HeapTuple      tup;
> +
> +        rel = table_open(PublicationSchemaRelationId, RowExclusiveLock);
> +
> +        tup = SearchSysCache1(PUBLICATIONSCHEMA, ObjectIdGetDatum(psoid));
> +
> +        if (!HeapTupleIsValid(tup))
> +                elog(ERROR, "cache lookup failed for publication schema %u",
> +                         psoid);
> +
> +        CatalogTupleDelete(rel, &tup->t_self);
> +
> +        ReleaseSysCache(tup);
> +
> +        table_close(rel, RowExclusiveLock);
> +}
>
> Since RemovePublicationSchemaById() does simple catalog tuple
> deletion, it seems to me that we can DropObjectById() to delete the
> row of pg_publication_schema.

Relation cache invalidations were missing in the function, I have added and retained the function with invalidation changes.

>  ---
>          {
> -                ScanKeyInit(&key[0],
> +                ScanKeyData skey[1];
> +
> +                ScanKeyInit(&skey[0],
>                                          Anum_pg_class_relkind,
>                                          BTEqualStrategyNumber, F_CHAREQ,
>
> CharGetDatum(RELKIND_PARTITIONED_TABLE));
>
> -                scan = table_beginscan_catalog(classRel, 1, key);
> +                scan = table_beginscan_catalog(classRel, 1, skey);
>
> Since we specify 1 as the number of keys in table_beginscan_catalog(),
> can we reuse 'key' instead of using 'skey'?

Modified.

> ---
> Even if we drop all tables added to the publication from it, 'pubkind'
> doesn't go back to 'empty'. Is that intentional behavior? If we do
> that, we can save the lookup of pg_publication_rel and
> pg_publication_schema in some cases, and we can switch the publication
> that was created as FOR SCHEMA to FOR TABLE and vice versa.

I felt this can be handled as a separate patch as the same scenario applies for all tables publication too. Thoughts?

> ---
> +static void
> +UpdatePublicationKindTupleValue(Relation rel, HeapTuple tup, int col,
> +                                                                char pubkind)
>
> Since all callers of this function specify Anum_pg_publication_pubkind
> to 'col', it seems 'col' is not necessary.

Modified

> ---
> +static void
> +AlterPublicationSchemas(AlterPublicationStmt *stmt, Relation rel,
> +                                                HeapTuple tup,
> Form_pg_publication pubform)
>
> I think we don't need to pass 'pubform' to this function since we can
> get it by GETSTRUCT(tup).

Modified.

> ---
> +                Oid                    schemaId = get_rel_namespace(relid);
>                  List      *pubids = GetRelationPublications(relid);
> +                List      *schemaPubids = GetSchemaPublications(schemaId);
>
> Can we defer to get the list of schema publications (i.g.,
> 'schemaPubids') until we find the PUBKIND_SCHEMA publication? Perhaps
> the same is true for building 'pubids'.

I felt that we can get the publication information and use it whenever required instead of querying in the loop. Thoughts?

> ---
> +                                                   List of publications
> +        Name        |          Owner           | All tables | Inserts
> | Updates | Deletes | Truncates | Via root | PubKind
> +--------------------+--------------------------+------------+---------+---------+---------+-----------+----------+---------
> + testpib_ins_trunct | regress_publication_user | f          | t
> | f       | f       | f         | f        | e
> + testpub_default    | regress_publication_user | f          | f
> | t       | f       | f         | f        | e
>
> I think it's more readable if \dRp shows 'all tables', 'table',
> 'schema', and 'empty' in PubKind instead of the single character.

Modified

> I think 'Pub kind' is more consistent with other column names.

Modified

Thanks for the comments, these issues are fixed in the v20 patch attached at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm00X9SQBokUTy1OxN1Sa2DFsK8rg8j_wLgc-7ZuKcuh0Q%40mail.gmail.com