Re: Added schema level support for publication.

On Thu, Aug 12, 2021 at 5:54 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
wrote:
>
> On Fri, Aug 6, 2021 at 5:33 PM vignesh C <vignesh21(at)gmail(dot)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

Regards,
Vignesh