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 1vNR4Q-002Vmj-3C for pgsql-hackers@arkaria.postgresql.org; Mon, 24 Nov 2025 07:33:11 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1vNR4P-00H3LO-20 for pgsql-hackers@arkaria.postgresql.org; Mon, 24 Nov 2025 07:33:09 +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 1vNR4P-00H3Ki-04 for pgsql-hackers@lists.postgresql.org; Mon, 24 Nov 2025 07:33:09 +0000 Received: from mail-qt1-x836.google.com ([2607:f8b0:4864:20::836]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1vNR4M-001ACB-0N for pgsql-hackers@lists.postgresql.org; Mon, 24 Nov 2025 07:33:08 +0000 Received: by mail-qt1-x836.google.com with SMTP id d75a77b69052e-4ed82e82f0fso39719671cf.1 for ; Sun, 23 Nov 2025 23:33:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1763969586; x=1764574386; 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=p7IWO+NzVv0TAPfNHEix+BMeOJI6X1WYH54OhHLPr6Q=; b=OKDJdlP6YkBGSntG72Ezuegxk+Qb4yCgXAkbnVtN2rezApVKRvCuaiWBaKAugmm7Ce MBxejLbKYQy8+7+tf8KDqIC0fD3iGGHVzffKXhlVcN8X/rx+t/1i2vyLpbaGPzz1FNiO m2JzI1z8doqRfjtndiKp+1Onc7DqXJ2mzXd3olk4ksmjWmM+m2NnfwKC1C8DtvKfLR6O yRjQvuI+O7kjLNQZOpdqdBRrdaT/LrWUok3OKhZFM0Dq4roOebKKlTvpyb0YU9zl+97/ 7sLAIE3sr619t8TZhfYKIvDKSViQJpCNs36+P6AQyaby/TvoD7Os6E16YHSeKg0Spkai TNvQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1763969586; x=1764574386; 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=p7IWO+NzVv0TAPfNHEix+BMeOJI6X1WYH54OhHLPr6Q=; b=M1uAgViqSWe7Fh3xRWB0IWEbiOWLLbtJkrgF+UHOP5g2ETCHEpBqptEKfnRAs91RtQ cZYys5TvZdllqTpKeeooICOZc6+2C4vn5UbHO87bqzEfB/CtaRHktxCvIQw23XGphEL7 PHgMX8YE7lm0utXhO3lL5O9zUZFGQhnCG5Vm4OlCT00TjavittMI3RyLS7Rb8VmfN5GJ aQnd0WeGrV4feDThPFCtCCjZMm+krib4LSylupSy34UAjmzTjk3u/QvdqfULjhY2BnX9 wRQfxXDc3PVavgTdVhDqfpyyPTJmVjgB8WIz+BvuenSw/9yST9pmlY7neyarAvytvfBL yFwg== X-Forwarded-Encrypted: i=1; AJvYcCXg/9FZNnu56tYTKDu7i/b+WrnEeMGjr0FNKD4t2Fk6TAtjjHhYBvVVasGWi9cIHpY28rT40tF48y/QAWKo@lists.postgresql.org X-Gm-Message-State: AOJu0Yw4Byq6dc9kFHYv/YbGkIWsrYfshv0DoiTHiCnb/s+vg3e3RoEK n9jQ/umBtY9TH3D5PXUQw2ij87mZl/DFumqQgPQkRd+euzK/e7+jhZwmgc/l5hhl9m+T+mBxrTa oelHeq04q6uMkYOanpMrMOunjMQXFx5w= X-Gm-Gg: ASbGncvjJ2uhm9TQIkyIC4nX68E2lyg5KAYbaT1roHkThIZp6c8K9G7GpOkSs3sbU+Y U82mXJ/eoBmJO8cmnz9tcrfYz+cdRMLqhCeXop/XYOnd+j/TJo5EP3F0syaOe48QjHQihPpMPG8 ZcC3uQfmzMoAazLq3m6fIW/lltV9mTEMo3qoBr1cDyTikHfrdr2VanF2X+c7x5p7CSjyllSJeVH /AVJphRXpLsS76h2gjPCqmc74PA35BJVgKhfSsg7g5Tuj0G5VKlfNcOBzjR2RpZJgOqYXithXd1 rIiIA63dsmeI1N0aFRgdnJOaFJGP8A== X-Google-Smtp-Source: AGHT+IEHk+yUKg3FhIhdO6zd3Pw2F+sF+Lum7aM7xyPC9BdMjKlghkXggn8/6pucukb8JZ6dhfKuiFBgMqgSTb00l9U= X-Received: by 2002:a05:622a:1394:b0:4ec:ee04:8831 with SMTP id d75a77b69052e-4ee589347e5mr154544391cf.57.1763969585833; Sun, 23 Nov 2025 23:33:05 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Peter Smith Date: Mon, 24 Nov 2025 18:32:39 +1100 X-Gm-Features: AWmQ_blvHvmY_jEdDNO_dp1Wj0G9jNjhbTjOj13ahPfCUuGyQyS32CCfBA6CKn8 Message-ID: Subject: Re: Skipping schema changes in publication To: Shlok Kyal Cc: vignesh C , Amit Kapila , "Zhijie Hou (Fujitsu)" , YeXiu <1518981153@qq.com>, Ian Lawrence Barwick , Bharath Rupireddy , PostgreSQL Hackers 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 Fri, Nov 21, 2025 at 5:55=E2=80=AFPM Peter Smith = wrote: > > Hi Shlok. > > Here are some review comments for your patch v28-0003 (EXCEPT TABLE ...). > > The review of this patch is a WIP. In this post I only looked at the test= code. > Here are my remaining review comments for patch v28-0003 (EXCEPT TABLE ...)= . =3D=3D=3D=3D=3D=3D doc/src/sgml/ref/create_publication.sgml 1. -ALTER PUBLICATION name ADD ALL TABLES +ALTER PUBLICATION name ADD ALL TABLES [ EXCEPT [ TABLE ] ( table_exception_object [, ... ] ) ] Why is that optional [TABLE] keyword needed? I know PostGres commands sometimes have "noise" words in the syntax so the command can be more English-like, but in this case, the publication is a FOR ALL *TABLES* anyway, so I am not sure what the benefit is of the user being able to say TABLE a 2nd time? =3D=3D=3D=3D=3D=3D src/backend/catalog/pg_publication.c 2. + /* + * Check for partitions of partitioned table which are specified with + * EXCEPT clause and partitioned table is published with + * publish_via_partition_root =3D true. + */ I think you can just say "partitions" or "table partitions", but "partitions of [a] partitioned table" seems overkill. Also, "... and partitioned table is published with publish_via_partition_root =3D true." seems too wordy. Isn't that just the same as "... and publish_via_partition_root =3D true" SUGGESTION Check for when the publication says "EXCEPT TABLE (partition)" but publish_via_partition_root =3D true. ~~~ 3. -/* Gets list of publication oids for a relation */ +/* Gets list of publication oids for a relation that matches the except_fl= ag */ List * -GetRelationPublications(Oid relid) +GetRelationPublications(Oid relid, bool except_flag) { List *result =3D NIL; CatCList *pubrellist; @@ -765,7 +791,8 @@ GetRelationPublications(Oid relid) HeapTuple tup =3D &pubrellist->members[i]->tuple; Oid pubid =3D ((Form_pg_publication_rel) GETSTRUCT(tup))->prpubid; - result =3D lappend_oid(result, pubid); + if (except_flag =3D=3D ((Form_pg_publication_rel) GETSTRUCT(tup))->prexce= pt) + result =3D lappend_oid(result, pubid); } I was wondering if it might be better to return 2 lists from this function (e.g. an included-list, and an excluded-list) instead of passing the 'except_flag' like the current code. IIUC, you are mostly calling this function twice to get 2 lists anyway, but returning 2 lists instead of 1, this function might be more efficient since it will only process the publication loop once. ~~~ 4. /* * Gets list of relation oids for a publication that matches the except_fla= g. * * This should only be used FOR TABLE publications, the FOR ALL TABLES/SEQU= ENCES * should use GetAllPublicationRelations(). */ List * GetPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt, bool except_flag) Something doesn't seem right -- the function comment says we shouldn't be calling the function for FOR ALL TABLES, but meanwhile, EXCEPT TABLE is currently only implemented via FOR ALL TABLES. So it feels contradictory. Maybe it is just the comment that needs updating? ~~~ 5. /* * Gets list of relation oids for a publication that matches the except_fla= g. * * This should only be used FOR TABLE publications, the FOR ALL TABLES/SEQU= ENCES * should use GetAllPublicationRelations(). */ List * GetPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt, bool except_flag) { List *result; Relation pubrelsrel; ScanKeyData scankey; SysScanDesc scan; HeapTuple tup; /* Find all publications associated with the relation. */ pubrelsrel =3D table_open(PublicationRelRelationId, AccessShareLock); Existing bug? Isn't this a bogus comment? /* Find all publications associated with the relation. */ Was that meant to be the other way around? -- e.g. Find all the relations associated with the specified publication. =3D=3D=3D=3D=3D=3D src/backend/commands/publicationcmds.c 6. + default: + /* shouldn't happen */ + elog(ERROR, "invalid publication object type %d", + puballobj->pubobjtype); + break; I think the ERROR is enough of a clue that it shouldn't happen. I felt the comment was redundant. ~~~ ObjectsInPublicationToOids: 7. case PUBLICATIONOBJ_TABLE: + pubobj->pubtable->except =3D false; + *rels =3D lappend(*rels, pubobj->pubtable); + break; + case PUBLICATIONOBJ_EXCEPT_TABLE: + pubobj->pubtable->except =3D true; *rels =3D lappend(*rels, pubobj->pubtable); break; Those are very similar. How about combining like below? 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; ~~ pub_contains_invalid_column: 8. pub_contains_invalid_column(Oid pubid, Relation relation, List *ancestors, bool pubviaroot, char pubgencols_type, - bool *invalid_column_list, + bool puballtables, bool *invalid_column_list, bool *invalid_gen_col) The 'pub_via_root' and 'pubgencols_type' are parameters. Somehow it seems more natural for the 'puballtables' to be passed before those, because FOR ALL TABLES comes before WITH in the syntax. ~~~ CreatePublication: 9. else if (!stmt->for_all_sequences) - { ObjectsInPublicationToOids(stmt->pubobjects, pstate, &relations, &schemaidlist); AFAICT, this function is refactored a lot because of the removal of that '{'. It looks like mostly whitespace, but really, I think the logic is quite different. I wasn't sure what that was about. Is it related to this patch, or some other bugfix in passing or what? =3D=3D=3D=3D=3D=3D src/backend/commands/tablecmds.c ATPrepChangePersistence: 10. - GetRelationPublications(RelationGetRelid(rel)) !=3D NIL) + list_length(GetRelationPublications(RelationGetRelid(rel), false)) > 0) Isn't an empty List the same as a NIL list? Maybe that list_length() change was not really needed. =3D=3D=3D=3D=3D=3D src/backend/parser/gram.y 11. drop_option_list pub_obj_list pub_all_obj_type_list + except_pub_obj_list opt_except_clause Is this name consistent with the others? Should it be pub_except_obj_list? ~~~ 12. %type PublicationObjSpec +%type ExceptPublicationObjSpec %type PublicationAllObjSpec Is this name consistent with the others? Should it be PublicationExceptObjS= pec? ~~~ CreatePublicationStmt: 13. n->pubname =3D $3; + n->pubobjects =3D $5; I noticed that sometimes there is a cast (List *) and other times there is not. e.g. none here, but cast in AlterPublicationStmt. Why the differences? ~~~ PublicationObjSpec: 14. The comment for 'PublicationObjSpec' says "FOR TABLE and FOR TABLES IN SCHEMA specifications". If that comment is correct, then why is this patch changing this code? OTOH, if the code is correct, then does the comment need updating? =3D=3D=3D=3D=3D=3D src/bin/pg_dump/pg_dump.c 15. Shouldn't there have already been some ALTER ... ADD ALL TABLE dump code and test code implemented back in patch 0002? ~~~ dumpPublication: 16. else if (pubinfo->puballtables) + { + SimplePtrListCell *cell; + appendPQExpBufferStr(query, " FOR ALL TABLES"); + + /* Include exception tables if the publication has except tables */ + for (cell =3D exceptinfo.head; cell; cell =3D cell->next) + { + PublicationRelInfo *pubrinfo =3D (PublicationRelInfo *) cell->ptr; + TableInfo *tbinfo; + + if (pubinfo =3D=3D pubrinfo->publication) + { + tbinfo =3D pubrinfo->pubtable; + + if (first) + { + appendPQExpBufferStr(query, " EXCEPT TABLE ("); + first =3D false; + } + else + appendPQExpBufferStr(query, ", "); + appendPQExpBuffer(query, "ONLY %s", fmtQualifiedDumpable(tbinfo)); + } + } + if (!first) + appendPQExpBufferStr(query, ")"); + } 16a. SimplePtrListCell *cell can be declared as a for-loop variable. ~ 16b. The comment should say "EXCEPT TABLES" in uppercase. ~ 16c. I am not convinced you can use that 'first' flag like you are doing. Isn't that interfering with the existing usage of that flag? Perhaps another boolean just for this EXCEPT loop is needed. ~~~ getPublicationTables: 17. + if (strcmp(prexcept, "f") =3D=3D 0) + pubrinfo[j].dobj.objType =3D DO_PUBLICATION_REL; + else + pubrinfo[j].dobj.objType =3D DO_PUBLICATION_EXCEPT_REL; + ... + if (strcmp(prexcept, "t") =3D=3D 0) + simple_ptr_list_append(&exceptinfo, &pubrinfo[j]); + Here you are comparing the same 'prexcept' flag for both "f" and "t". I felt it was better if both comparisons are the same (e.g. both "t"). Or better still, assign a new boolean and avoid that 2nd strcmp entirely -- e.g. except_flag =3D (strcmp(prexcept, "t") =3D=3D 0); =3D=3D=3D=3D=3D=3D src/bin/pg_dump/pg_dump_sort.c DOTypeNameCompare: 18. + else if (obj1->objType =3D=3D DO_PUBLICATION_EXCEPT_REL) + { + PublicationRelInfo *probj1 =3D *(PublicationRelInfo *const *) p1; + PublicationRelInfo *probj2 =3D *(PublicationRelInfo *const *) p2; + + /* Sort by publication name, since (namespace, name) match the rel */ + cmpval =3D strcmp(probj1->publication->dobj.name, + probj2->publication->dobj.name); + if (cmpval !=3D 0) + return cmpval; + } Isn't this identical to the previous code block? So can't you just add DO_PUBLICATION_EXCEPT_REL to that condition? =3D=3D=3D=3D=3D=3D src/bin/pg_dump/t/002_pg_dump.pl 19. Missing test cases for ALTER? But also. ~~~ 20. Missing test cases for EXCEPT for INHERITED tables? =3D=3D=3D=3D=3D=3D src/bin/psql/describe.c describeOneTableDetails: 21. I was wondering if the "describe" for tables (e.g. \d+) should also show the publications where the table is an ECEPT TABLE? How else is the user going to know it has been excluded by some publication? =3D=3D=3D=3D=3D=3D src/bin/psql/tab-complete.in.c ALTER PUBLICATION: 22. The tab completion does not seem as good as it could be. e.g, there is missing '(' and the for EXCEPT TABLE ~~~ CREATE PUBLICATION: 23. The tab completion does not seem as good as it could be. e.g, there is missing '(' and the for EXCEPT TABLE =3D=3D=3D=3D=3D=3D src/test/regress/sql/publication.sql 24. +\dRp+ testpub_foralltables_excepttable +\dRp+ testpub_foralltables_excepttable1 As well as doing the "describes" for the publication, I think we need to see the test cases for the describes of those excluded tables. e.g. I imagine that they should also list the publications that they are *excluded* from, right? ~~~ 25. +CREATE PUBLICATION testpub5 FOR ALL TABLES EXCEPT TABLE (testpub_tbl3); +CREATE PUBLICATION testpub6 FOR ALL TABLES EXCEPT TABLE (ONLY testpub_tbl3= ); 25a. Needs some explanatory comments here saying these are for testing the EXCEPT with inherited tables (e.g. ONLY versus not). ~ 25b. I think you should be testing the '*' syntax here too. ~~~ 26. +CREATE TABLE pub_sch1.tbl2 (a int); SET client_min_messages =3D 'ERROR'; CREATE PUBLICATION testpub_reset FOR ALL TABLES, ALL SEQUENCES; RESET client_min_messages; @@ -1344,9 +1358,15 @@ ALTER PUBLICATION testpub_reset ADD ALL TABLES; -- Can't add ALL TABLES to 'ALL TABLES' publication ALTER PUBLICATION testpub_reset ADD ALL TABLES; +ALTER PUBLICATION testpub_reset RESET; + +-- Verify adding EXCEPT TABLE +ALTER PUBLICATION testpub_reset ADD ALL TABLES EXCEPT TABLE (pub_sch1.tbl1, pub_sch1.tbl2); +\dRp+ testpub_reset DROP PUBLICATION testpub_reset; DROP TABLE pub_sch1.tbl1; +DROP TABLE pub_sch1.tbl2; DROP SCHEMA pub_sch1; It looks like that added CREATE TABLE (and RESET?) belongs more appropriately within the scope of the new test "Verify adding EXCEPT TABLE". =3D=3D=3D=3D=3D=3D Kind Regards, Peter Smith. Fujitsu Australia