On Fri, Feb 20, 2026 at 5:59 AM Peter Smith <[email protected]> wrote:
>
> A few of the recent function name change suggestions have originated
> from my internal review comments. But, these changes are not very
> effective unless they are done across existing functions as well to
> keep everything consistent.
>
> Background
> ----------
> TBH, I always found many of the current HEAD names to be confusing. I
> am often having to double-check back with the function comment to find
> what was the intention.
>
> e.g. Here is a list of some existing function names of pg_publication.c.
> GetPubPartitionOptionRelations
> GetRelationPublications
> GetPublicationRelations
> GetAllTablesPublications
> GetAllPublicationRelations
> GetPublicationSchemas
> GetSchemaPublications
> GetSchemaPublicationRelations
> GetAllSchemaPublicationRelations
> GetPublication
> GetPublicationByName
>
> At the core of my confusion is that the pattern GetYyyyXxxx means 2
> different things:
>
> - Sometimes the Yyyy is a search condition or "criteria" for what you
> are getting. e.g. GetRelationPublications means "Get publications that
> the specified relid is a member of"
>
> - sometimes the Yyyy is more like an "attribute" for what you are
> getting. e.g. GetAllTablesPublications means "Get all of the FOR ALL
> TABLES publications"
>
> Often there is some ambiguity:
>
> e.g GetAllPublicationRelations
> - does it mean "Get relations list equivalent to a FOR ALL TABLES
> publication"?
> - does it mean "Get relations that are members of all the specified
> publications"?
>
> e.g. GetSchemaPublications
> - does it mean "Get publications marked as FOR TABLES IN SCHEMA"?
> - does it mean "Get publications for the specified schemaid"?
>
> etc.
>
> ~~~
>
> This thread's EXCEPT patch is not causing new problems, it's just
> adding yet more functions to the long list of those that I felt are
> ambiguous.
>
> IMO, when a function returns XXX, then the function name should be
> prefixed GetXXX.
>
> So, I would advocate for existing function names to change; something
> like below.
>
> GetPubPartitionOptionRelations -> GetRelsByRelidAndPartitionOpt
> GetRelationPublications -> GetPubsByRelid
> GetPublicationRelations -> GetRelsByPubidMarkedForTable
> GetAllTablesPublications -> GetPubsMarkedForAllTables
> GetAllPublicationRelations -> GetRelsOfPubsMarkedForAll
> GetPublicationSchemas -> GetSchemasByPubidMarkedForTablesInSchema
> GetSchemaPublications -> GetPubsMarkedForTablesInSchemaBySchemaid
> GetSchemaPublicationRelations -> GetPublishedRelsBySchemaid
> GetAllSchemaPublicationRelations -> GetRelsOfPubsMarkedForTablesInSchema
> GetPublication -> GetPubByPubid
> GetPublicationByName -> GetPubByName
>
> Then, any new (non-static) functions for this EXCEPT patch would be
> named similarly.
>
> ~
>
> I understand that maybe everyone else feels the current names are fine
> as-is and not confusing at all.
> Or, maybe the confusion is recognised, but now is just not a good time
> of year to change everything.
>
> Anyway, I just wanted to give the background, and explain reasons for
> those function name suggestions.
>
Few more comments on top of these:
+ elog(LOG, "fetch_relation_list: executing query to fetch
effectiverelations: \n%s",
+ cmd.data);
This looks like a debugging log message, did you add it for debugging
purposes but later forgot to change its elevel or probably clean up?
--
+relation_is_effectively_excluded(Oid relid, List *exceptlist)
+{
+ List *leaftables = NIL;
+
+ if (exceptlist == NIL)
+ return false;
+
+ /* Explicitly excluded */
+ if (list_member_oid(exceptlist, relid))
+ return true;
+
+ /* Get all leaf partitions of relid */
+ leaftables = GetPubPartitionOptionRelations(leaftables,
+ PUBLICATION_PART_LEAF,
+
"leaftables" are never freed on either return path. Since this
function is called in a loop from GetAllPublicationRelations() during
catalog scans, this has the potential to accumulate memory.
--
+ res = walrcv_exec(LogRepWorkerWalRcvConn, cmd.data,
+ lengthof(filtertableRow), filtertableRow);
Seems like we are missing "walrcv_clear_result()" for this.
--
+/* Helper: Check syscache for prexcept flag */
+bool
+is_relid_excepted(Oid relid, Oid pubid)
The comment atop this function needs some improvement it seems. It is
trying to explain how it is implemented, rather than its purpose.
--
With Regards,
Ashutosh Sharma,
