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 1vXZ5K-00FAzM-39 for pgsql-hackers@arkaria.postgresql.org; Mon, 22 Dec 2025 06:07:59 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1vXZ5I-00DRTN-2e for pgsql-hackers@arkaria.postgresql.org; Mon, 22 Dec 2025 06:07:57 +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 1vXZ5I-00DRTE-10 for pgsql-hackers@lists.postgresql.org; Mon, 22 Dec 2025 06:07:57 +0000 Received: from mail-qt1-x831.google.com ([2607:f8b0:4864:20::831]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1vXZ5H-001v87-1d for pgsql-hackers@lists.postgresql.org; Mon, 22 Dec 2025 06:07:56 +0000 Received: by mail-qt1-x831.google.com with SMTP id d75a77b69052e-4eda26a04bfso44814471cf.2 for ; Sun, 21 Dec 2025 22:07:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1766383674; x=1766988474; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=RhTi1qOTQoSiBHllpbRks+kDpc9Lel7MI5YOnIyiUG4=; b=UPhBBkEyQQVkzK66AuC6xJQCT8x6Fs/8ZOGtLxbpoQgE6jI8UHGZc3SyJhkzbjwvVM 8OJhAC0msB+TrvBglX4cpPAcdnk6aLwREswsqirDWLTv4YEizVLlFf0lYS5VqMl6P5W+ 0O6jM0sebXF2yLn3E77MpPWiyCMtLmCjGyQW2uNrJ4+9r602+Iheu91yvLzPLe3oHrLs pauhnjKxDd5zUNZFtg716eE0Cjv1+HWQAGg1d8d8soxhNgBoQ2qOTs1nrAS9seMCi2dX /uAB/olLUB8+6p7zJ8zoB4gvTcNRBNymbvmR9dGIvUm65oiX0Tk/sd3TW3i3RhEEdOax W0Pw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1766383674; x=1766988474; h=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=RhTi1qOTQoSiBHllpbRks+kDpc9Lel7MI5YOnIyiUG4=; b=Mka0u1bRjsfRkpqwKsdbIq5WykBLwCDmE3sVRgrSejX2A4bTUlOy0AcgPzelxiHiP2 ULD5JMc+EphmgrLVhppS1V2mMHdCEWE7Tgn1H1IGm39YuogDjLn3Dxjugtj0eg35GN0h THFVoJtEglGTcV6KS/Ixeu+gbTVSAeeyHTHyou8SQ6311d1Qjf0MjcjpxCD2pSo/cTvO UxvePgpyLrm1QyT+/9ogy3kHwg+5nQuGrV1vphIUS+olnsE/sf5qfL1BErH/mjhBwA5y cBPVV06UOOxSAutfUaX9tcp99vgIr0OzuvaqLnrFtAa0MMIYyAhxzBiQvqRoicSs0Eau opsw== X-Forwarded-Encrypted: i=1; AJvYcCUkh3rL2uXh9Qa99g7858XYQwz9/tisSlS96Wjy5v6wBMLbfotBt9HoAfGk4RLFmu8jh1lnTK8jaD85Rr3D@lists.postgresql.org X-Gm-Message-State: AOJu0YzBSv8v6Xx1rzmb+TJI2Hpk+IiE+OzPM7RP1AhnrCs7poqLCENc wQYUQsPwZmDnrVeMtsfFdSZKJSOfS0UiNlg/PpSBQkvaeIUdR8YzdmGIoo4dcfMiN19vW2wUf0y hbjPy9p5IAnGk0Z94xQ1qHADei4yfDAk= X-Gm-Gg: AY/fxX555UuAjbYygpHXoVqaPaqcwYM5e4M0vvNWdhTyrf5+bksO9Uc0u1+PjKKFjAN D2kxs3URO9ZizgQPQvRKLdeOBRR9h2f6fRf4uS3ZhmVzQVrWFoDjMaU3o2DCZW2zm0anXDnv3Zw Av8DKchm4nV112eiZzQEv1JdLdZi6TKYIOaynwc1oDNB2JYZiodMEEODuj3LjOeXpNDLE/2Hp6G vVfEJB8hUy5JV0aB5Bsi5Cd9NPrpM5DGO975uM/1U5JSqmviT0biFwuiJiiS8mmukomces7DOvt CM6dFgbMLpbx4zBKSUp6tIW1VQdpFA== X-Google-Smtp-Source: AGHT+IH+3p3D528cpfo/AtD/VycaJ0CRWxcZtQgx6gOloOv2RTlL+KuHn3wBJnZCXYpMHhrDCm7ljxZBbvQ4UGSDQZs= X-Received: by 2002:a05:622a:4ccc:b0:4e2:e58a:57e1 with SMTP id d75a77b69052e-4f4abd1fd20mr168376131cf.37.1766383674126; Sun, 21 Dec 2025 22:07:54 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Peter Smith Date: Mon, 22 Dec 2025 17:07:27 +1100 X-Gm-Features: AQt7F2rDadKU_d1Sqx-1Epd_Rk7jjDwj0pp93djka5X1j1vZye4Zqqp8eXTQGT8 Message-ID: Subject: Re: Skipping schema changes in publication To: Shlok Kyal Cc: shveta malik , Amit Kapila , vignesh C , "Zhijie Hou (Fujitsu)" , YeXiu <1518981153@qq.com>, Ian Lawrence Barwick , Bharath Rupireddy , PostgreSQL Hackers Content-Type: text/plain; charset="UTF-8" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk Hi Shlok. Some review comments for patch v33-0001 (code part) ====== src/backend/catalog/pg_publication.c GetPublicationRelationsInternal: 1. Static function names should be snake_case. ~~~ GetPublicationIncludedRelations: 2. +/* + * Return the list of relation OIDs for a publication. + * + * For a FOR TABLE publication, this returns the list of relations explicitly + * included in the publication. + * + * Publications declared with FOR ALL TABLES or FOR ALL SEQUENCES should use + * GetAllPublicationRelations() to obtain the complete set of tables covered by + * the publication. + */ +List * +GetPublicationIncludedRelations(Oid pubid, PublicationPartOpt pub_partopt) +{ + Assert(!GetPublication(pubid)->alltables); + + return GetPublicationRelationsInternal(pubid, pub_partopt, false); +} Why isn't the Assert also saying something about puballsequences, as mentioned in the function comment? ~~~ GetAllPublicationRelations: 3. + * 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 SEQUENCES + * publications. 3. It seems wrong to say "FOR ALL SEQUENCES" ... that seems to assume the "FOR ALL SEQUENCES" and "FOR ALL TABLES" cannot co-exist. Did you mean "Neither of ... to published sequences"? ~ 4. -GetAllPublicationRelations(char relkind, bool pubviaroot) +GetAllPublicationRelations(Oid pubid, char relkind, bool pubviaroot) There are tricky rules about relation vs sequences and the publish_via_partition_root parameter value. It would be better if you encapsulate all this within this function. Specifically, it would be simpler if you passed the 'Publication' arg instead of the pubid. Then you can get the pubviaroot value from that (within the function) instead of passing around "fake" values of false when you are looking at RELKIND_SEQUENCE. ====== src/backend/commands/publicationcmds.c ObjectsInAllPublicationToOids: 5. + foreach_ptr(PublicationAllObjSpec, puballobj, puballobjspec_list) + { + if (puballobj->pubobjtype != PUBLICATION_ALL_TABLES) + continue; + + foreach_ptr(PublicationObjSpec, pubobj, puballobj->except_tables) + { + pubobj->pubtable->except = true; + *rels = lappend(*rels, pubobj->pubtable); + } + } I think it's tidier to code this like below: if (puballobj->pubobjtype == PUBLICATION_ALL_TABLES) { foreach_ptr... } ~~~ pub_contains_invalid_column: 6. bool pub_contains_invalid_column(Oid pubid, Relation relation, List *ancestors, bool pubviaroot, char pubgencols_type, - bool *invalid_column_list, - bool *invalid_gen_col) + bool *invalid_column_list, bool *invalid_gen_col) Why does this change even exist at all in this patch? ~~~ CreatePublication: 7. + /* + * If the publication is for ALL TABLES and 'relations' is not empty, it + * indicates that some relations should be excluded from the publication. + * 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. + */ + if (relations != NIL) + { + List *rels; + + rels = OpenTableList(relations); + TransformPubWhereClauses(rels, pstate->p_sourcetext, + publish_via_partition_root); + + CheckPubRelationColumnList(stmt->pubname, rels, + schemaidlist != NIL, + publish_via_partition_root); + + PublicationAddTables(puboid, rels, true, NULL); + CloseTableList(rels); + } + The comment and the code don't match. The comment is talking about rules for FOR ALL TABLES, but puballtables is not part of any condition here (??). Was all this supposed to be within the "if (stmt->for_all_tables)" code block? ====== src/bin/pg_dump/pg_dump.c 8. - "SELECT tableoid, oid, prpubid, prrelid, " + "SELECT tableoid, oid, prpubid, prrelid,\n" "pg_catalog.pg_get_expr(prqual, prrelid) AS prrelqual, " "(CASE\n" " WHEN pr.prattrs IS NOT NULL THEN\n" @@ -4868,6 +4929,9 @@ getPublicationTables(Archive *fout, TableInfo tblinfo[], int numTables) " WHERE attrelid = pr.prrelid AND attnum = prattrs[s])\n" " ELSE NULL END) prattrs " "FROM pg_catalog.pg_publication_rel pr"); + if (fout->remoteVersion >= 190000) + appendPQExpBufferStr(query, " WHERE prexcept = false"); 8a Isn't it better to qualify everything here with the alias 'pr'? ~ 8b. Also "WHERE NOT pr.prexcept;" might be more conssitent with other code I saw in describe.c ====== src/bin/pg_dump/pg_dump.h 9. PublishGencolsType pubgencols_type; + SimplePtrList excepttbls; } PublicationInfo; How about "tables instead of "tbls" (e.g. "excepttables" or "except_tables") here? That would also be more consistent with the other puballtables member. ====== src/test/regress/sql/publication.sql 10. RESET client_min_messages; \dRp+ testpub3 \dRp+ testpub4 +\dRp+ testpub5 +\dRp+ testpub6 +\dRp+ testpub7 I feel it would be better to keep each \dRp+ together with the test it belongs to, rather than have a bunch of different tests which are then followed by a bunch of different \dRp+. Note: this same comment applies to other place of places -- not just here. Check everywhere you do \dRp+ ====== Kind Regards, Peter Smith. Fujitsu Australia