Re: Support EXCEPT for ALL SEQUENCES publications

On Fri, 31 Jul 2026 at 07:28, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Here are some review comments for the v22* patches.
>
> //////////
> Patch 0001
> //////////
>
> src/backend/commands/publicationcmds.c
>
> OpenRelationList:
>
> 1.
> static List *
> -OpenTableList(List *tables)
> +OpenRelationList(List *relations)
>
> The static declaration of OpenRelationList and the associated
> CloseRelationList function called this parameter `rels`, so it might
> be better to stay consistent with that.
>
> //////////
> Patch 0002
> //////////
>
> On Thu, Jul 30, 2026 at 10:50 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> ...
>
> > > EXCEPT:
> > >
> > > 3.
> > > Nisha's EXCEPT thread docs [2;patch 0005] have an EXCEPT parameter on this page.
> > > e.g.
> > > + <varlistentry>
> > > + <term><literal>EXCEPT</literal></term>
> > >
> > > I think you should do the same, because that will make merge/rebase
> > > easier later when both these patches eventually get pushed. Please
> > > refer to the other thread to make yours similar.
> > >
> > I am not sure why an EXCEPT parameter is need on
> > alter_publication.sgml. We have already described the behaviour of
> > EXCEPT with ALL SEQUENCE and ALL TABLES in a paragraph earlier in the
> > same page.
> > Also we have described the EXCEPT parameter in
> > create_publication.sgml. Won't it be redundant to add the same
> > description again?
>
> 1.
> Here are some reasons:
>
> a. IMO the current ALTER page's "Description" is already unwieldy in
> comparison to other pages (compare it to CREATE PUBLICATION), so I
> predict it will need a make-over sometime (that can be much later
> after both these threads are done). So adding EXCEPT now is just a
> small step in that direction.
>
> b. Description is already large, but Nisha's SCHEMA EXCEPT patch
> needed to add much more to it, so instead we opted for a Parameter
> EXCEPT in that thread to keep it manageable.
>
> c. There is no need for redundant information. Of course, ALTER ...
> EXCEPT can xref refer back to CREATE ... EXCEPT for common
> information, so it's not just a cut/paste of content. But there are
> some special considerations for ALTER to explain -- e.g. only way to
> modify exclusions is to use SET, and that doing so completely
> overwrites any pre-existing exclusions etc.
>
> Anyway, you can skip adding an EXCEPT parameter if you disagree. But
> AFAIK, the SCHEMA EXCEPT patch is likely to be pushed ahead of your
> EXCEPT SEQUENCE one, so you will have to rebase accordingly anyway.
> I'm just suggesting it's better to try to stay compatible with Nisha's
> patch now, instead of having to rebase too much later.
>
I agree with your points, I have added an EXCEPT parameter.

> ======
> src/backend/commands/publicationcmds.c
>
> get_delete_rels:
>
> 2.
> + /*
> + * Validate the column list. If the column list or WHERE clause
> + * changes, then the validation done here will be duplicated
> + * inside PublicationAddRelations(). The validation is cheap
> + * enough that that seems harmless.
> + */
> + if (newrelid == oldrelid &&
> + equal(oldwhereexpr, newpubrel->whereClause))
> + {
> + Bitmapset *newcolumns = pub_collist_validate(newpubrel->relation,
> + newpubrel->columns);
> +
> + if (bms_equal(oldcolumns, newcolumns))
> + {
> + found = true;
> + break;
> + }
> + }
>
> The comment saying "Validate the column list" seems premature.
>
> The outer comment should be explaining the need to compare row filters
> and column lists. If you want to describe more about column list
> validation then that should be a comment *inside* the if where that
> validation happens.

I have also addressed the remaining comments and attached an updated v23 patch.

Thanks,
Shlok Kyal