Re: Added schema level support for publication.

On Thu, Jun 17, 2021 at 12:41 AM vignesh C <vignesh21@gmail.com> wrote:

> Thanks for reporting it, the attached patch is a rebased version of
> the patch with few review comment fixes, I will reply with the comment
> fixes to the respective mails.
>

I've applied the patch, it applies cleand and ran "make check" and
tests run fine.

Some comments for patch 1:

In the commit message, some grammar mistakes:

"Changes was done in
pg_dump to handle pubtype updation in pg_publication table while the database
gets upgraded."

-------------- change to --

Changes were done in
pg_dump to handle pubtype updation in pg_publication table while the database
gets upgraded.

=============

Prototypes present in pg_publication.h was moved to publicationcmds.h so
that minimal datastructures ...

----------------- change to --

Prototypes present in pg_publication.h were moved to publicationcmds.h so
that minimal datastructures ..

========================

In patch 1:

In getObjectDescription()

+ if (!nspname)
+ {
+ pfree(pubname);
+ pfree(nspname);
+ ReleaseSysCache(tup);

Why free nspname if it is NULL?

Same comment in getObjectIdentityParts()
============================

In GetAllTablesPublicationRelations()

+ ScanKeyData key[2];
  TableScanDesc scan;
  HeapTuple tuple;
  List    *result = NIL;
+ int keycount = 0;

  classRel = table_open(RelationRelationId, AccessShareLock);

- ScanKeyInit(&key[0],
+ ScanKeyInit(&key[keycount++],

Here you have init key[1], but the code below in the same function
inits key[0]. I am not sure if this will affect that code below.

if (pubviaroot)
{
ScanKeyInit(&key[0],
Anum_pg_class_relkind,
BTEqualStrategyNumber, F_CHAREQ,
CharGetDatum(RELKIND_PARTITIONED_TABLE));
=================================

in UpdatePublicationTypeTupleValue():

+ tup = heap_modify_tuple(tup, RelationGetDescr(rel), values, nulls,
+ replaces);
+
+ /* Update the catalog. */
+ CatalogTupleUpdate(rel, &tup->t_self, tup);

Not sure if this tup needs to be freed or if the memory context will
take care of it.
=====================

regards,
Ajin Cherian
Fujitsu Australia