| From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
|---|---|
| To: | Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> |
| Cc: | shveta malik <shveta(dot)malik(at)gmail(dot)com>, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Support EXCEPT for ALL SEQUENCES publications |
| Date: | 2026-08-03 07:44:49 |
| Message-ID: | CAHut+Pt9tYPJ2sgBZ2d-NQAA8f2=H8oN5mGK6Rro5FppPPbsiQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Some review comments for v23-0002
======
doc/src/sgml/ref/alter_publication.sgml
1.
+ publication defined for <literal>ALL SEQUENCES</literal>. When
+ <literal>EXCEPT</literal> is specified with
+ <literal>SET ALL TABLES</literal> or
+ <literal>SET ALL SEQUENCES</literal>, it replaces the existing exclusion
+ list; if omitted, the existing exclusion list is cleared. See
+ <xref linkend="sql-alterpublication-params-except"/> for more details.
The whole part "When ... for more details." maybe can be removed
because now the EXCEPT parameter is saying this same information.
~~~
2.
+ <varlistentry id="sql-alterpublication-params-except">
+ <term><literal>EXCEPT</literal></term>
+ <listitem>
+ <para>
+ When used with <literal>SET ALL TABLES</literal> or
+ <literal>SET ALL SEQUENCES</literal>, specifies the tables or sequences
+ to be excluded from the publication. The specified tables or sequences
+ replaces any existing exclusions. If <literal>EXCEPT</literal>
is omitted,
+ any existing exclusions are cleared.
+ </para>
For the first sentence, is it better to use the same wording as on the
CREATE PUBLICATION page?
SUGGESTION
Specifies the tables to be excluded from an ALL TABLES publication, or
the sequences to be excluded from an ALL SEQUENCES publication.
======
src/backend/commands/publicationcmds.c
get_delete_rels:
1.
+ /*
+ * Validate the column list. If the column list changes, then
+ * the validation done here will be duplicated inside
+ * PublicationAddRelations(). The validation is cheap enough
+ * that that seems harmless.
+ */
+ Bitmapset *newcolumns = pub_collist_validate(newpubrel->relation,
+ newpubrel->columns);
That "Validate the column list." comment seems slightly misleading
because I don't think that is the real purpose here. IIUC, we don't
really want to do validation; we're calling this function to get the
Bitmapset, and the validation is just a side-effect.
SUGGESTION
Get the column list Bitmapset. Validation is a side effect and will be
duplicated inside PublicationAddRelations() if the column list
changed. That's cheap enough that it seems harmless.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Chao Li | 2026-08-03 07:47:09 | Re: A new C function `get_partition_root`. |
| Previous Message | ZizhuanLiu X-MAN | 2026-08-03 07:23:14 | Re: [PATCH] Doc: Mention OFF as an alias for EXPLAIN SERIALIZE NONE |