Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1vXYDy-00EgOI-0Q for pgsql-hackers@arkaria.postgresql.org; Mon, 22 Dec 2025 05:12:51 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1vXYDw-00DHQB-1x for pgsql-hackers@arkaria.postgresql.org; Mon, 22 Dec 2025 05:12:49 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1vXYDw-00DHQ0-09 for pgsql-hackers@lists.postgresql.org; Mon, 22 Dec 2025 05:12:49 +0000 Received: from mail-pj1-x1034.google.com ([2607:f8b0:4864:20::1034]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1vXYDu-001ugQ-2h for pgsql-hackers@lists.postgresql.org; Mon, 22 Dec 2025 05:12:47 +0000 Received: by mail-pj1-x1034.google.com with SMTP id 98e67ed59e1d1-34c27d14559so2771483a91.2 for ; Sun, 21 Dec 2025 21:12:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1766380365; x=1766985165; darn=lists.postgresql.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=xroGGE/9VIj9kKpfcMc0gGZc2MHMgFgVAhPZSxgJ7Vk=; b=VBdcpyggAEyWYiPb+Vq7MMj3lc2HK6XS90cxsjXrQsI5aW/80wZIPC7Gd43eZeR3ns 9SwSJzAoMLHEag/9RgQkUATDBTDuHUQbT9ndSqHnjxRr69bziWT17qTL6F8N2xSshsAh TQ35sM14jH23jG+T+jur+R3O2U1XS5rj17XXaXpeMfMRuWaJhX4BHD9CD5PpirFnnbKp bD3k5c/vno9oVyHFPM1UEvly5nlC/apF0/Bp4dVW/lAH/fYZ+KHHK/BvAaTiWOgDkzby AasDWMWM8cNd481SBlcLRSkrAI/g+nbmPgfjENX3nGRw6dE32JW37JYdAdCBZMOmOAIW JqQg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1766380365; x=1766985165; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=xroGGE/9VIj9kKpfcMc0gGZc2MHMgFgVAhPZSxgJ7Vk=; b=SGlbEYNOHPJWh77GQ0fD2C5cZgBI3sKpbsf/kaGSJXes3y4ZL94Z2vNiVHjF/ZgTmu tl1Exv/j+gdUw5F0Kn+zIdjNrWmrK8oFQdC1q55NeZHzGp+tnRFZxsSbDb41+zf2QNq4 7TJAkGZv/ExlGuxqpZrXvyQd8xO4QbcdkIV0KipkFL5AMutM8YeiNtQDkpaULucLObPE dO+xFDaAspN1bjhS02kMksIdAVFPIVkzq3IPJpaSJWZRCnGJw8bPvhkqpSShNHckBNrm XXuuK2k7aCZwk3Km3NQ90OfF1MdARmbSxQ/6gTujfUUQoCL1r6nvBahq8FxoFVau9DsG ju3g== X-Forwarded-Encrypted: i=1; AJvYcCWD0wc9HaO1rqzx76ifE7QH8uvVkW+6qkntHBa3F7qwUUR33iMprhL2ficvFN3az+zBmofgOw8zKHOViBlN@lists.postgresql.org X-Gm-Message-State: AOJu0Yx4pk3gwDAL44eMHW3mPr34yQvsG0lSafRzOnkrEy3YwJmnCZrO RmbaItmcEzD0POKwz+kxsPskIGwh2SsGnhkrdq94vxYp5AoNslvVIHDORosTK3/ZfccwCmiM4fD 4L86lESpWC1HmKXfBuYj/bXDPLI3QO/s= X-Gm-Gg: AY/fxX4yzD7ANMMf/V6P1AUKdQ1EDl+aYpwIV29WPvgvgvAnN293PAAElOiMWU77a/P rMeuv2xLckF3iWt8brFp3urkJOj/J8pA8+bukh7Bef9sUStPWQy5ZgR98ZlZKlxG9rKRiak5P1R rUnOho+7pI0bWivXpvuhSLBlGcMbA2bEysv/G9iPxyw7jD4bXNHGiRZM++ZCxNiY1PP3wQWS+Yu hMMOBw0DJIgG0KAVaKwr4v/C7F1LrvX3jkaQbd6qylCZ6RNeOnBZ0KB0GLpIcFdn0kC+08Eyp3W 2IJyObtDebbbA0Ve61CCKGACzB6ZIzxYTTO1JsRG+w== X-Google-Smtp-Source: AGHT+IGVgsvN3UqARLjuNd9majAyAN+yCDsT7hp8LWcLD2Yr0Qmtz/7XSD18x/NqZYKj3A8tj7GNZk8H/VzrbSxzPaM= X-Received: by 2002:a17:90b:548c:b0:341:124f:474f with SMTP id 98e67ed59e1d1-34e921e8a0fmr7298591a91.32.1766380365417; Sun, 21 Dec 2025 21:12:45 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: shveta malik Date: Mon, 22 Dec 2025 10:42:33 +0530 X-Gm-Features: AQt7F2poQNzlsHpUyYH0NDy5qN64pfLvWuZStLtER4e3dqAmqQ2xCUlYLjgNEG0 Message-ID: Subject: Re: Skipping schema changes in publication To: Shlok Kyal Cc: Peter Smith , Amit Kapila , vignesh C , "Zhijie Hou (Fujitsu)" , YeXiu <1518981153@qq.com>, Ian Lawrence Barwick , Bharath Rupireddy , PostgreSQL Hackers , shveta malik Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk On Thu, Dec 18, 2025 at 5:15=E2=80=AFPM Shlok Kyal wrote: > > On Thu, 18 Dec 2025 at 11:30, shveta malik wrote= : > > > > On Wed, Dec 17, 2025 at 11:24=E2=80=AFAM shveta malik wrote: > > > > > > On Tue, Dec 16, 2025 at 2:50=E2=80=AFPM Shlok Kyal wrote: > > > > > > > > I have also addressed the remaining comments and attached the lates= t patch. > > > > > > > > > > Thanks. A few comments: > > > > > > 1) > > > + if (!set_top && puballtables) > > > + set_top =3D !list_member_oid(aexceptpubids, puboid); > > > > > > In GetTopMostAncestorInPublication(), we have made the above change > > > which will now get ancestor from all-tables publication as well, > > > provided table is not part of 'except' List. Earlier this function wa= s > > > only checking pg_subscription_rel and pg_publication_namespace which > > > does not include all-tables publication. Won't it change the > > > result-set for callers? > > > > It can change the result set of callers. I analysed more and saw that > GetTopMostAncestorInPublication is called from 3 places. > 1. pub_rf_contains_invalid_column: it is called when publication is > not ALL TABLES. It will have no impact with the change. > 2. pub_contains_invalid_column : it is called for all type of > publication. it calls GetTopMostAncestorInPublication like: > ``` > if (pubviaroot && relation->rd_rel->relispartition) > { > publish_as_relid =3D GetTopMostAncestorInPublication(pubid, ancestors= , > NULL, puballtables); > > if (!OidIsValid(publish_as_relid)) > publish_as_relid =3D relid; > } > ``` > In HEAD for ALL TABLES publication GetTopMostAncestorInPublication > will always return InvalidOid. With this patch it can have some value. > So there is a difference in behaviour. > > 3. get_rel_sync_entry > in HEAD we had > ``` > if (pub->alltables) > { > publish =3D true; > if (pub->pubviaroot && am_partition) > { > List *ancestors =3D get_partition_ancestors(relid); > > pub_relid =3D llast_oid(ancestors); > ancestor_level =3D list_length(ancestors); > } > } > ``` > With patch this condition is not valid because we cannot set > 'pub_relid =3D llast_oid(ancestors);' directly as the table can be > excluded. > So, the change in GetTopMostAncestorInPublication will even handle the > case of "ALL TABLES" publication. > > Since we have a behaviour difference for the 2nd function, I have > removed the changes for 'ALL TABLES' from > GetTopMostAncestorInPublication and added it separately > 'get_rel_sync_entry'. Thoughts? I find the current implementation better, the previous one was impacting the results of different paths. Regarding: + if (list_member_oid(aexceptpubids, puboid)) + { + list_free(aexceptpubids); + continue; + } IMO, if puboid is part of apubids, that check is enough. This is because aexceptpubids and apubids are mutually exclusive lists for a particular 'ancestor'. But if we want to have it to avoid schma-mapping check later, we should add a comment. How about: This step isn=E2=80=99t strictly necessary, but we keep it so we can skip t= he table if it appears in the EXCEPT list, avoiding an expensive schema-mapping check later. > > > > 2) > > > + * Publications declared with FOR ALL TABLES or FOR ALL SEQUENCES sh= ould use > > > + * GetAllPublicationRelations() to obtain the complete set of tables= covered by > > > + * the publication. > > > + */ > > > +List * > > > +GetPublicationIncludedRelations(Oid pubid, PublicationPartOpt pub_pa= rtopt) > > > +{ > > > + return GetPublicationRelationsInternal(pubid, pub_partopt, false); > > > +} > > > > > > We can have an Assert here that pubid passed is not for ALL-Tables or > > > ALL-sequences > > > > Added assert for all tables. I found during testing that this function > can be called for ALL SEQUENCES in HEAD. So I have not added an > assertion for it. > I think it is a bug and shared the same in [1]. Will add assert for > all sequences as well once the bug is fixed. > Okay. > > > 3) > > > GetAllPublicationRelations: > > > * If the publication publishes partition changes via their respectiv= e root > > > * partitioned tables, we must exclude partitions in favor of includi= ng the > > > * root partitioned tables. This is not applicable to FOR ALL SEQUENC= ES > > > * publication. > > > > > > + * The list does not include relations that are explicitly excluded = via the > > > + * EXCEPT TABLE clause of the publication specified by pubid. > > > > > > Suggestion: > > > /* > > > * If the publication publishes partition changes via their respectiv= e root > > > * partitioned tables, we must exclude partitions in favor of includi= ng the > > > * root partitioned tables. The list also excludes tables that are > > > * explicitly excluded via the EXCEPT TABLE clause of the publication > > > * identified by pubid. Neither of these rules applies to FOR ALL SEQ= UENCES > > > * publications. > > > */ > > > > > > 4) > > > GetAllPublicationRelations: > > > + if (relkind =3D=3D RELKIND_RELATION) > > > + exceptlist =3D GetPublicationExcludedRelations(pubid, pubviaroot ? > > > + PUBLICATION_PART_ALL : > > > + PUBLICATION_PART_ROOT); > > > > > > Assert(!(relkind =3D=3D RELKIND_SEQUENCE && pubviaroot)); > > > > > > Generally we keep such parameters' sanity checks as the first step. W= e > > > can add new code after Assert. > > > > > > 5) > > > ObjectsInAllPublicationToOids() only has one caller which calls it > > > under check: 'if (stmt->for_all_tables)' > > > > > > Thus IMO, we do not need a switch-case in > > > ObjectsInAllPublicationToOids(). We can simply have a sanity check to > > > see it is 'PUBLICATION_ALL_TABLES' and then do the needed operation > > > for this pub-type. > > > > > > 6) > > > CreatePublication(): > > > /* > > > * If publication is for ALL TABLES and relations is not empty, it mea= ns > > > * that there are some relations to be excluded from the publication. > > > * Else, relations is the list of relations to be added to the > > > * publication. > > > */ > > > > > > Shall we rephrase slightly to: > > > > > > /* > > > * If the publication is for ALL TABLES and 'relations' is not empty, > > > * it indicates that some relations should be excluded from the publi= cation. > > > * Add those excluded relations to the publication with 'prexcept' se= t to true. > > > * Otherwise, 'relations' contains the list of relations to be explic= itly > > > * included in the publication. > > > */ > > > > > > 7) > > > + /* Associate objects with the publication. */ > > > + if (stmt->for_all_tables) > > > + { > > > + /* Invalidate relcache so that publication info is rebuilt. */ > > > + CacheInvalidateRelcacheAll(); > > > + } > > > > > > I think this comment is misplaced. We shall have it at previous place= , atop: > > > if (stmt->for_all_tables) > > > This is because here we are just trying to invalidate cache while at > > > previous place we are trying to associate. > > > > > > > Few more: > > > > 8) > > get_rel_sync_entry() > > + List *exceptTablePubids =3D NIL; > > > > At all other places, we are using exceptpubids, shall we use the same h= ere? > > > > 9) > > ObjectsInPublicationToOids() > > > > case PUBLICATIONOBJ_TABLE: > > + case PUBLICATIONOBJ_EXCEPT_TABLE: > > + pubobj->pubtable->except =3D (pubobj->pubobjtype =3D=3D > > PUBLICATIONOBJ_EXCEPT_TABLE); > > *rels =3D lappend(*rels, pubobj->pubtable); > > break; > > > > It looks slightly odd that for pubobjtype case > > 'PUBLICATIONOBJ_EXCEPT_TABLE', we have to check pubobjtype against > > PUBLICATIONOBJ_EXCEPT_TABLE itself. > > > > Shall we make it: > > case PUBLICATIONOBJ_EXCEPT_TABLE: > > pubobj->pubtable->except =3D true; > > /* fall through */ > > case PUBLICATIONOBJ_TABLE: > > *rels =3D lappend(*rels, pubobj->pubtable); > > break; > > > We should also make pubobj->pubtable->except =3D false for PUBLICATIONOBJ= _TABLE? yes, right. > Updated the condition like: > case PUBLICATIONOBJ_EXCEPT_TABLE: > pubobj->pubtable->except =3D true; > *rels =3D lappend(*rels, pubobj->pubtable); > break; > case PUBLICATIONOBJ_TABLE: > pubobj->pubtable->except =3D false; > *rels =3D lappend(*rels, pubobj->pubtable); > break; > Looks good. > > 10) > > I want to understand the usage of DO_PUBLICATION_EXCEPT_REL. Can you > > give a scenario where its usage in DOTypeNameCompare() will be hit? > > Its all other usages too need some analysis and validation. > > > In the current patch we are not setting an objecttype to > DO_PUBLICATION_EXCEPT_REL. > We are storing the list of except tables in 'pubinfo[i].excepttbls' > list in function getPublications and "pubinfo[i].dobj.objType =3D > DO_PUBLICATION". So, I don't see any requirement of > DO_PUBLICATION_EXCEPT_REL now. I have removed it. > Yes, that was my initial thought as well, that we might not need it. But I=E2=80=99ll review it further and let you know. > > 11) > > + List *except_objects; /* List of publication object to be excluded= */ > > > > object --> objects > > Currently since we exclude only tables, does it make sense to name it > > as except_tables? > > > I have also addressed the remaining comments and attached the updated v33= patch. > [1]: https://www.postgresql.org/message-id/CALDaNm0qoNtsX%2B9KPug6qb%3DuC= -H2iPMYW%2BgL%3DHehx%2BNgOxga6w%40mail.gmail.com > Thanks, will review. thanks Shveta