On Wed, Apr 29, 2026 at 12:02 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Nisha.
>
> A couple of ad-hoc review comments for v4-0001...
>
> ======
> src/bin/pg_dump/pg_dump.c
>
> dumpPublicationNamespace:
>
> 1.
> + if (!first_except)
> + appendPQExpBufferStr(query, ")");
>
> The logic seems ok, but the above seems a bit strange, checking
> 'first_except'. Maybe renaming to 'has_except' (and some small
> refactoring) would read better?
>
> ~~~
Thanks for pointing that out. This also needed updating after the
syntax change to EXCEPT (TABLE ...).
I’ve used the name "has_except", defaulting it to false for better readability.
>
> 2.
> IIUC, the 'ALTER PUBLICATION' EXCEPT clause syntax change is not
> introduced until your patch 0003. So, how does this dump code even
> work when it relies on that ALTER PUBLICATION?
>
> Furthermore, the patch 0003 commit message says 'ALTER PUBLICATION pub
> SET TABLES', but this dump code is using 'ALTER PUBLICATION pub ADD
> TABLES' (note ADD v SET). Something seems suspicious...
>
I think there was a slight mix-up in reading the patch messages. The
ALTER PUBLICATION ... EXCEPT syntax is introduced in patch-002 for
ADD, not patch-003.
That said, you’re right. The pg_dump part doesn’t work with patch-001
alone. I missed moving it to patch-002 while splitting the patches.
Thanks for catching that; it’s now fixed.
Also, patch-003 is for SET (not ADD) and includes handling for cleanup
of EXCEPT entries when a schema is dropped. I’ve updated the patch
message for clarity.
Attached v5 patches with these updates.
--
Thanks,
Nisha
