On Fri, 7 Aug 2026 at 16:20, shveta malik <[email protected]> wrote: > > Agree with the problem statement and the approach. Please find a few > initial comments, will review in detail next week. > > 1) > getObjectTypeDescription: > > + if (isPublicationRelationExcept(object->objectId, missing_ok)) > + appendStringInfoString(&buffer, "publication exclusion"); > + else > + appendStringInfoString(&buffer, "publication relation"); > > It seems strange initially that when missing_okay is true and say > cache-tuple is missing, we ccnsider it as 'publication relation'. But > then I checked other calls accepting 'missing_ok' in the same function > such as: getProcedureTypeDescription, getConstraintTypeDescription. > They have a fallback option for undefined object. The comment there > makes it clear. Perhaps we should add similar comment here.
Comment seems sufficient here, added a comment. > 2) > It seems that get_object_address_publication_rel() currently performs > two cache lookups to obtain the complete details of the relation. > Ideally, a single lookup would suffice, although that would likely > require restructuring the implementation instead of going through > isPublicationRelationExcept(). That said, I understand that > isPublicationRelationExcept() is still needed in other code paths, so > even though I would prefer a single cache lookup, keeping the current > approach is fine as well. I preferred the single lookup approach. The v1-0002-Review-comment-fixes.patch at [1] has the changes for the same. [1] - https://www.postgresql.org/message-id/CALDaNm14MGg8cw3WXyDyt-ry2rh4ifVbsNRVUnwJZcBmxVo%2BRg%40mail.gmail.com Regards, Vignesh
