On Thu, Aug 12, 2021 at 5:54 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Fri, Aug 6, 2021 at 5:33 PM vignesh C <vignes...@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 Regards, Vignesh