On Mon, Jun 1, 2026 at 12:59 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Nisha.
>
> Some review comments for patch v8-0004.
>
Thanks for the review.
> ======
> src/backend/commands/publicationcmds.c
>
> AlterPublicationSchemaExceptTables:
>
> 1.
> + /* Collect OIDs of the desired new EXCEPT list. */
> + foreach_ptr(PublicationRelInfo, pri, rels)
> + {
> + newexceptrelids = lappend_oid(newexceptrelids,
> + RelationGetRelid(pri->relation));
> + }
>
> Block braces {} not needed.
>
Fixed.
> ~~~
>
> 2.
> + if (!OidIsValid(proid))
> + continue; /* already gone */
> +
> + ObjectAddressSet(obj, PublicationRelRelationId, proid);
> + performDeletion(&obj, DROP_CASCADE, 0);
>
> SUGGESTION
> if (OidIsValid(proid))
> {
> ObjectAddressSet(obj, PublicationRelRelationId, proid);
> performDeletion(&obj, DROP_CASCADE, 0);
> }
>
Fixed. A similar pattern was also used in PublicationDropSchemas(),
and I have fixed that as well.
> ======
> src/test/subscription/t/037_except.pl
>
> 3.
> I think you had used the SQL exactly as I previously suggested, but I
> made a mistake:
> It should say "SELECT count(*)" instead of "SELECT a".
>
> So it returns either 0 or 1 row.
>
> e.g. #1
> $result =
> $node_subscriber->safe_psql('postgres',
> "SELECT count(*) FROM sch1.tab_excluded WHERE a = 7");
> is($result, qq(1),
> 'ALTER ... SET TABLES IN SCHEMA EXCEPT: newly included table is replicated'
> );
> $result =
> $node_subscriber->safe_psql('postgres',
> "SELECT count(*) FROM sch1.tab_published WHERE a = 7");
> is($result, qq(0),
> 'ALTER ... SET TABLES IN SCHEMA EXCEPT: now-excluded table is not
> replicated'
> );
>
> e.g. #2
> $result =
> $node_subscriber->safe_psql('postgres',
> "SELECT count(*) FROM sch1.tab_published WHERE a = 8");
> is($result, qq(1),
> 'ALTER ... SET TABLES IN SCHEMA (no EXCEPT): tab_published
> replicated after except list cleared'
> );
> $result =
> $node_subscriber->safe_psql('postgres',
> "SELECT count(*) FROM sch1.tab_excluded WHERE a = 8");
> is($result, qq(1),
> 'ALTER ... SET TABLES IN SCHEMA (no EXCEPT): tab_excluded
> replicated after except list cleared'
> );
>
Thanks for pointing that out; I overlooked your earlier suggestion.
I've now updated as suggested.
Attached is the updated v9 patch set.
--
Thanks,
Nisha
