Re: Added schema level support for publication.

Thanks for the comments.

On Fri, Jun 18, 2021 at 5:25 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Thu, Jun 17, 2021 at 12:41 AM vignesh C <vignesh21(at)gmail(dot)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.
>

I will modify this.

> =============
>
> 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 ..
>
> ========================

I will modify this.

>
> In patch 1:
>
> In getObjectDescription()
>
> + if (!nspname)
> + {
> + pfree(pubname);
> + pfree(nspname);
> + ReleaseSysCache(tup);
>
> Why free nspname if it is NULL?
>
> Same comment in getObjectIdentityParts()

I will modify this.

> ============================
>
> 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));

I felt this is ok as we specify the keycount to be 1, so only the
key[0] will be used. Thoughts?
ScanKeyInit(&key[0],
Anum_pg_class_relkind,
BTEqualStrategyNumber, F_CHAREQ,
CharGetDatum(RELKIND_PARTITIONED_TABLE));

scan = table_beginscan_catalog(classRel, 1, key);

> =================================
>
> 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.

I felt this is ok, as the cleanup is handled in the caller function
"AlterPublication", thoughts?
/* Cleanup. */
heap_freetuple(tup);
table_close(rel, RowExclusiveLock);

I will post an update patch for the fixes shortly.

Regards,
Vignesh