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
