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 1vW74D-00DRni-0z for pgsql-hackers@arkaria.postgresql.org; Thu, 18 Dec 2025 06:00:50 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1vW74B-000OtS-0b for pgsql-hackers@arkaria.postgresql.org; Thu, 18 Dec 2025 06:00:47 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1vW74A-000OtK-2k for pgsql-hackers@lists.postgresql.org; Thu, 18 Dec 2025 06:00:47 +0000 Received: from mail-pj1-x102c.google.com ([2607:f8b0:4864:20::102c]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1vW749-001MOD-04 for pgsql-hackers@lists.postgresql.org; Thu, 18 Dec 2025 06:00:47 +0000 Received: by mail-pj1-x102c.google.com with SMTP id 98e67ed59e1d1-34c565c3673so662658a91.0 for ; Wed, 17 Dec 2025 22:00:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1766037642; x=1766642442; 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=RHaCy7i5n6e73BGK8TsNExS9DCYn5iqPSZCI2I3hL3M=; b=CIV/AImpDsUMZeSPLexhSa/65eXmj+RJEJBaXJ8am6qWqeUzmFfO1YrQsvoyFrd00U IzfBIdxB0baddtHyCtdYJ++xMkf3AmpBEz53YDrvcyBQ8c/GMVGWIovP7KIpqofLDrEO d8wKVGm+LBarrAblkpFLeySaXsms2sAZcERx3eWGPASrH/lb/JhLNeALhrT+McFBbpOf meaRSMOXLlBs7T/e+Dghh2ffx1wKfQmB5et7thwD6PpDYU9VL9ExgK/epqBnxDR0Hhq7 7yaagoduWlDQMXgHRzR/lbQpA6IVaprHOhd0+/y6g296WWoJVeh8bwhl0pkrwhoTOuLm zPGA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1766037642; x=1766642442; 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=RHaCy7i5n6e73BGK8TsNExS9DCYn5iqPSZCI2I3hL3M=; b=Mkv5H5bJQFfCn6OhKISWzNUYUL5dwoCcICB+hbeUf/D4z5i3F08Se+ixuzAY2qtGUE sSgQ21QxaMF5w9tAPPZmrx8X4m3egIqdfJ7Pv4VKukB72bLxxdxq9z9S/YpXWC72y9B6 wkj9Eb6CRziWKlpL0K01bCCVWuWxU2d6i0aRfUFdUGXK2Luvad1PvpRSnspAmbeUC46O o+0Im0IhHFUk0PywmmK8LZnMjCrNgcojIwyhwWCJLoKck3e9O/MNRtS8J7VEX8lTVgmm F4KVcVv1CJo91Mkqg9hquH3EDbCYedbd9e2uHU1CqQRnxvcJ1anhdnlHRw2ymxW51u5o NEYg== X-Forwarded-Encrypted: i=1; AJvYcCVXefpBnJ479xeFJ+gm3YSK3P5Hh4ZfOhB+AKZ56xze//sL7m4coO+v0fRfXoxCR0vqsv7n6BDT/DtfkJ57@lists.postgresql.org X-Gm-Message-State: AOJu0Yx5h4Tb5GrgLbiDQxHy+SD6BKoTUTEtJD9M+9v+mxQ1RNlB5bxE Y5IarXwAYOdRGLUo2u6PaHFDhmTVmG0edXcKie8h6X2dxZgc/sIAzGroxI/dO6evFFhPzKZ27Fn VF89vNcQdn3jmzHxQ8m1uCSueyDaLOw0= X-Gm-Gg: AY/fxX6kdlMq/+9gt3wlbv5fpDSiadStaUjHmPGRKxDiUBum7uFqzMA5HxPDQrACyKX PBAhK1nfa3lGq0ZiiUw7XIHqkqWnt8XV9Qf4Z0DEmvfaOA2IgYEgdDP+SquT79ukCdE/IxfYyre 8KUcst/MDcJhZqGZX71lL6wrq2ufSVPZ8nK8NNGQbEkQbFihcXGfTRRUkotopZlvEtdPiQfpp3v OqxXKHYYlA9d8LKetOZkS+f8fwKzr35DXSgrpBPGn2iu/vRhVk+CQ6rnQSe0+uMCBYH9hS8kQSH VAwZ3sv6nsMokoxyt5H0yYETGsSPyCJ6a5oq9zZyFg== X-Google-Smtp-Source: AGHT+IFy9e3bJIs0+4pql9zQyUSCA1gy2o/LTh7JXcsWwForjEIZBG5Y5ISINGBIEgTZmNrgM8EQQrDsi1rnEngGK/I= X-Received: by 2002:a17:90b:4b0b:b0:340:c094:fbff with SMTP id 98e67ed59e1d1-34e71e09fecmr1588210a91.10.1766037642283; Wed, 17 Dec 2025 22:00:42 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: shveta malik Date: Thu, 18 Dec 2025 11:30:30 +0530 X-Gm-Features: AQt7F2rX1cM6v_XkEo0yFIrBZJTJ_nTgbDzt4edCEG8a-iNELSPrTLQKyYOITig 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 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 latest pa= tch. > > > > 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 was > 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? > > 2) > + * Publications declared with FOR ALL TABLES or FOR ALL SEQUENCES should= use > + * GetAllPublicationRelations() to obtain the complete set of tables cov= ered by > + * the publication. > + */ > +List * > +GetPublicationIncludedRelations(Oid pubid, PublicationPartOpt pub_partop= t) > +{ > + return GetPublicationRelationsInternal(pubid, pub_partopt, false); > +} > > We can have an Assert here that pubid passed is not for ALL-Tables or > ALL-sequences > > 3) > GetAllPublicationRelations: > * If the publication publishes partition changes via their respective ro= ot > * partitioned tables, we must exclude partitions in favor of including t= he > * root partitioned tables. This is not applicable to FOR ALL SEQUENCES > * 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 respective ro= ot > * partitioned tables, we must exclude partitions in favor of including t= he > * 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 SEQUENC= ES > * 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. We > 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 means > * 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 publicati= on. > * Add those excluded relations to the publication with 'prexcept' set to= true. > * Otherwise, 'relations' contains the list of relations to be explicitly > * 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, at= op: > 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 here? 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; 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. 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? thanks Shveta