Thread

Re: Skipping schema changes in publication

Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-12-09T17:49:26Z

On Fri, 21 Nov 2025 at 12:26, Peter Smith <smithpb2250@gmail.com> 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.
>
> ======
> .../t/037_rep_changes_except_table.pl
>
> 1.
> +
> +# Copyright (c) 2021-2025, PostgreSQL Global Development Group
> +
> +# Logical replication tests for except table publications
>
> Use uppercase: /except table/EXCEPT TABLE/
>
> ~~~
>
> 2.
> There are lots of test cases dedicated to partiion-table testing. I
> felt a bigger comment separating these major groups might be helpful.
>
> Something like:
>
> -- ============================================
> -- EXCEPT TABLE test cases for normal tables
> -- ============================================
>
> and
>
> -- ============================================
> -- EXCEPT TABLE test cases for partition tables
> -- ============================================
>
> ~~~
>
> 3.
> +# Initialize publisher node
> ...
> +# Create subscriber node
>
> Those 2 comments should be almost alike -- e.g. both should say
> "Initialize" or both should say "Create".
>
> ~~~
>
> 4.
> +# Test replication with publications created using FOR ALL TABLES EXCEPT TABLE
> +# clause.
> +# Create schemas and tables on publisher
> +$node_publisher->safe_psql(
> + 'postgres', qq(
> + CREATE SCHEMA sch1;
> + CREATE TABLE sch1.tab1 AS SELECT generate_series(1,10) AS a;
> + CREATE TABLE public.tab1(a int);
> +));
> +
>
> That first sentence ("Test replication with ...") is not needed here.
> The is just repeating the purpose of the entire file, so that comment
> can replace the one at the top of this file.
>
> ~~~
>
> 5.
> +# Insert some data and verify that inserted data is not replicated
>
> Be explicit that we are referring to the excluded table.
>
> SUGGESTION (e.g.)
> Verify that data inserted to the excluded table is not replcated.
>
> ~~~
>
> 6.
> +# Alter publication to exclude data changes in public.tab1 and verify that
> +# subscriber does not get the changed data for this table.
> +$node_publisher->safe_psql(
> + 'postgres', qq(
> + ALTER PUBLICATION tap_pub_schema RESET;
> + ALTER PUBLICATION tap_pub_schema ADD ALL TABLES EXCEPT TABLE
> (sch1.tab1, public.tab1);
> + INSERT INTO public.tab1 VALUES(generate_series(1,10));
> +));
> +$node_publisher->wait_for_catchup('tap_sub_schema');
> +
>
> It is not strictly needed for these tests, but do you think it makes
> more sense to also do an ALTER SUBSCRIPTION ... REFRESH PUBLICATION;
> whenever you change the publications?
>
> ~~~
>
> 7.
> +# cleanup
> +$node_publisher->safe_psql('postgres', "DROP PUBLICATION tap_pub_schema");
> +$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION tap_sub_schema");
> +
> +
>
> double-blank lines.
>
> ~~~
>
> 8.
> I think it would be more helpful if the partition table test cases say
> (in their comments) a lot more about the steps they are doing, and
> what they expect the result to be. Sure, I can read all the code to
> figure it out for each case, but it is better to know the test
> intentions/expectations then verify they are doing the right thing.
>
> ~~~
>
> 9.
> + CREATE TABLE sch1.t1(a int) PARTITION BY RANGE(a);
> + CREATE TABLE sch1.part1 PARTITION OF sch1.t1 FOR VALUES FROM (0) TO (5);
>
> Maybe create this table to have *multiple* partitions. It might be
> interesting later to see what happens when you try to EXCEPT only one
> of the partitions.
>
I have addressed all the comments
Please find the updated patch in [1].

[1]: https://www.postgresql.org/message-id/CANhcyEXwLrQsec6g%2B1dqWTKyJQMQMh%3Dgetj28C%2BzLL14BjuumA%40mail.gmail.com

Thanks,
Shlok Kyal