CREATE SUBSCRIPTION - add missing test case

Started by Peter Smithover 1 year ago11 messages

smithpb2250@gmail.com

over 1 year ago

1 attachment(s)

Hi Hackers,

While reviewing another logical replication thread [1]/messages/by-id/CAHut+Pt5FqV7J9GnnWFRNW_Z1KOMMAZXNTRcRNdtFrfMBz_GLA@mail.gmail.com, I found an
ERROR scenario that seems to be untested.

TEST CASE: Attempt CREATE SUBSCRIPTION where the subscriber table is
missing some expected column(s).

Attached is a patch to add the missing test for this error message.

======
[1]: /messages/by-id/CAHut+Pt5FqV7J9GnnWFRNW_Z1KOMMAZXNTRcRNdtFrfMBz_GLA@mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

Attachments:

v1-0001-Add-missing-test-case.patchapplication/octet-stream; name=v1-0001-Add-missing-test-case.patchDownload

From 90cfe272a96ab87f128f85f95db21220b7e9e772 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Thu, 15 Aug 2024 17:17:03 +1000
Subject: [PATCH v1] Add missing test case

---
 src/test/subscription/t/004_sync.pl | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/src/test/subscription/t/004_sync.pl b/src/test/subscription/t/004_sync.pl
index a2d9462..8c3630e 100644
--- a/src/test/subscription/t/004_sync.pl
+++ b/src/test/subscription/t/004_sync.pl
@@ -172,6 +172,41 @@ ok( $node_publisher->poll_query_until(
 		'postgres', 'SELECT count(*) = 0 FROM pg_replication_slots'),
 	'DROP SUBSCRIPTION during error can clean up the slots on the publisher');
 
+# clean up
+$node_publisher->safe_psql('postgres', "DROP TABLE tab_rep");
+$node_subscriber->safe_psql('postgres', "DROP TABLE tab_rep");
+
+##########
+# TEST:
+#
+# When a subscriber table has a column missing that was specified on
+# the publisher table.
+##########
+
+# setup structure with existing data on publisher
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_rep (a int, b int, c int)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_rep VALUES (1, 1, 1), (2, 2, 2), (3, 3, 3)");
+
+# add table on subscriber; note columns 'b' and 'c' are missing
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_rep (a int)");
+my $offset = -s $node_subscriber->logfile;
+
+# create the subscription
+$node_subscriber->safe_psql('postgres',
+	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub"
+);
+
+# confirm missing column error is reported
+$node_subscriber->wait_for_log(
+	qr/ERROR: ( [A-Z0-9]+:)? logical replication target relation "public.tab_rep" is missing replicated columns: "b", "c"/,
+	$offset);
+
+# clean up
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION tap_sub");
+$node_subscriber->safe_psql('postgres', "DROP TABLE tab_rep");
+$node_publisher->safe_psql('postgres', "DROP TABLE tab_rep");
+
 $node_subscriber->stop('fast');
 $node_publisher->stop('fast');
 
-- 
1.8.3.1

vignesh C

vignesh21@gmail.com

over 1 year ago

In reply to: Peter Smith (#1)

Re: CREATE SUBSCRIPTION - add missing test case

On Thu, 15 Aug 2024 at 12:55, Peter Smith <smithpb2250@gmail.com> wrote:

Hi Hackers,

While reviewing another logical replication thread [1], I found an
ERROR scenario that seems to be untested.

TEST CASE: Attempt CREATE SUBSCRIPTION where the subscriber table is
missing some expected column(s).

Attached is a patch to add the missing test for this error message.

I agree currently there is no test to hit this code. I'm not sure if
this is the correct location for the test, should it be included in
the 008_diff_schema.pl file? Additionally, the commenting style for
this test appears quite different from the others. Could we use a
commenting style consistent with the earlier tests?

Regards,
Vignesh

Peter Smith

smithpb2250@gmail.com

over 1 year ago

In reply to: vignesh C (#2)

1 attachment(s)

Re: CREATE SUBSCRIPTION - add missing test case

On Fri, Aug 16, 2024 at 2:15 PM vignesh C <vignesh21@gmail.com> wrote:

Thanks for the review.

I agree currently there is no test to hit this code. I'm not sure if
this is the correct location for the test, should it be included in
the 008_diff_schema.pl file?

Yes, that is a better home for this test. Done as suggested in
attached patch v2.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

Attachments:

v2-0001-Add-missing-test-case.patchapplication/octet-stream; name=v2-0001-Add-missing-test-case.patchDownload

From c4254c6490223557d7e434962d585bc158dfcade Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Tue, 20 Aug 2024 12:46:13 +1000
Subject: [PATCH v2] Add missing test case

---
 src/test/subscription/t/008_diff_schema.pl | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/src/test/subscription/t/008_diff_schema.pl b/src/test/subscription/t/008_diff_schema.pl
index cf04f85..51ebfe0 100644
--- a/src/test/subscription/t/008_diff_schema.pl
+++ b/src/test/subscription/t/008_diff_schema.pl
@@ -118,6 +118,20 @@ is( $node_subscriber->safe_psql(
 	qq(1|1|1),
 	'check replicated inserts on subscriber');
 
+# Test if the expected error is reported when the subscriber table is missing
+# columns which were specified on the publisher table.
+
+$node_publisher->safe_psql('postgres', "CREATE TABLE test_tab3 (a int, b int, c int)");
+$node_subscriber->safe_psql('postgres', "CREATE TABLE test_tab3 (a int)");
+
+my $offset = -s $node_subscriber->logfile;
+
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub REFRESH PUBLICATION");
+
+$node_subscriber->wait_for_log(
+	qr/ERROR: ( [A-Z0-9]+:)? logical replication target relation "public.test_tab3" is missing replicated columns: "b", "c"/,
+	$offset);
 
 $node_subscriber->stop;
 $node_publisher->stop;
-- 
1.8.3.1

vignesh C

vignesh21@gmail.com

over 1 year ago

In reply to: Peter Smith (#3)

Re: CREATE SUBSCRIPTION - add missing test case

On Tue, 20 Aug 2024 at 08:21, Peter Smith <smithpb2250@gmail.com> wrote:

On Fri, Aug 16, 2024 at 2:15 PM vignesh C <vignesh21@gmail.com> wrote:

Thanks for the review.

I agree currently there is no test to hit this code. I'm not sure if
this is the correct location for the test, should it be included in
the 008_diff_schema.pl file?

Yes, that is a better home for this test. Done as suggested in
attached patch v2.

Thanks, this looks good to me.

Regards,
Vignesh

Amit Kapila

amit.kapila16@gmail.com

over 1 year ago

In reply to: vignesh C (#2)

Re: CREATE SUBSCRIPTION - add missing test case

On Fri, Aug 16, 2024 at 9:45 AM vignesh C <vignesh21@gmail.com> wrote:

On Thu, 15 Aug 2024 at 12:55, Peter Smith <smithpb2250@gmail.com> wrote:

Hi Hackers,

While reviewing another logical replication thread [1], I found an
ERROR scenario that seems to be untested.

TEST CASE: Attempt CREATE SUBSCRIPTION where the subscriber table is
missing some expected column(s).

Attached is a patch to add the missing test for this error message.

I agree currently there is no test to hit this code.

I also don't see a test for this error condition. However, it is not
clear to me how important is it to cover this error code path. This
code has existed for a long time and I didn't notice any bugs related
to this. There is a possibility that in the future we might break
something because of a lack of this test but not sure if we want to
cover every code path via tests as each additional test also has some
cost. OTOH, If others think it is important or a good idea to have
this test then I don't have any objection to the same.

--
With Regards,
Amit Kapila.

Peter Smith

smithpb2250@gmail.com

over 1 year ago

In reply to: Amit Kapila (#5)

Re: CREATE SUBSCRIPTION - add missing test case

On Wed, Aug 21, 2024 at 8:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Aug 16, 2024 at 9:45 AM vignesh C <vignesh21@gmail.com> wrote:

On Thu, 15 Aug 2024 at 12:55, Peter Smith <smithpb2250@gmail.com> wrote:

Hi Hackers,

While reviewing another logical replication thread [1], I found an
ERROR scenario that seems to be untested.

TEST CASE: Attempt CREATE SUBSCRIPTION where the subscriber table is
missing some expected column(s).

Attached is a patch to add the missing test for this error message.

I agree currently there is no test to hit this code.

I also don't see a test for this error condition. However, it is not
clear to me how important is it to cover this error code path. This
code has existed for a long time and I didn't notice any bugs related
to this. There is a possibility that in the future we might break
something because of a lack of this test but not sure if we want to
cover every code path via tests as each additional test also has some
cost. OTOH, If others think it is important or a good idea to have
this test then I don't have any objection to the same.

Yes, AFAIK there were no bugs related to this; The test was proposed
to prevent accidental future bugs.

BACKGROUND

Another pending feature thread (replication of generated columns) [1]/messages/by-id/B80D17B2-2C8E-4C7D-87F2-E5B4BE3C069E@gmail.com
required many test combinations to confirm all the different expected
results which are otherwise easily accidentally broken without
noticing. This *current* thread test shares one of the same error
messages, which is how it was discovered missing in the first place.

~~~

PROPOSAL

I think this is not the first time a logical replication test has been
questioned due mostly to concern about creeping "costs".

How about we create a new test file and put test cases like this one
into it, guarded by code like the below using PG_TEST_EXTRA [2]https://www.postgresql.org/docs/devel/regress-run.html#REGRESS-ADDITIONAL?

Doing it this way we can have better code coverage and higher
confidence when we want it, but zero test cost overheads when we don't
want it.

e.g.

src/test/subscription/t/101_extra.pl:

if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bsubscription\b/)
{
plan skip_all =>
'Due to execution costs these tests are skipped unless subscription
is enabled in PG_TEST_EXTRA';
}

# Add tests here...

======
[1]: /messages/by-id/B80D17B2-2C8E-4C7D-87F2-E5B4BE3C069E@gmail.com
[2]: https://www.postgresql.org/docs/devel/regress-run.html#REGRESS-ADDITIONAL

Kind Regards,
Peter Smith.
Fujitsu Australia

Peter Smith

smithpb2250@gmail.com

over 1 year ago

In reply to: Peter Smith (#6)

Re: CREATE SUBSCRIPTION - add missing test case

On Thu, Aug 22, 2024 at 8:54 AM Peter Smith <smithpb2250@gmail.com> wrote:

On Wed, Aug 21, 2024 at 8:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Aug 16, 2024 at 9:45 AM vignesh C <vignesh21@gmail.com> wrote:

On Thu, 15 Aug 2024 at 12:55, Peter Smith <smithpb2250@gmail.com> wrote:

Hi Hackers,

While reviewing another logical replication thread [1], I found an
ERROR scenario that seems to be untested.

TEST CASE: Attempt CREATE SUBSCRIPTION where the subscriber table is
missing some expected column(s).

Attached is a patch to add the missing test for this error message.

I agree currently there is no test to hit this code.

I also don't see a test for this error condition. However, it is not
clear to me how important is it to cover this error code path. This
code has existed for a long time and I didn't notice any bugs related
to this. There is a possibility that in the future we might break
something because of a lack of this test but not sure if we want to
cover every code path via tests as each additional test also has some
cost. OTOH, If others think it is important or a good idea to have
this test then I don't have any objection to the same.

Yes, AFAIK there were no bugs related to this; The test was proposed
to prevent accidental future bugs.

BACKGROUND

Another pending feature thread (replication of generated columns) [1]
required many test combinations to confirm all the different expected
results which are otherwise easily accidentally broken without
noticing. This *current* thread test shares one of the same error
messages, which is how it was discovered missing in the first place.

~~~

PROPOSAL

I think this is not the first time a logical replication test has been
questioned due mostly to concern about creeping "costs".

How about we create a new test file and put test cases like this one
into it, guarded by code like the below using PG_TEST_EXTRA [2]?

Doing it this way we can have better code coverage and higher
confidence when we want it, but zero test cost overheads when we don't
want it.

e.g.

src/test/subscription/t/101_extra.pl:

if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bsubscription\b/)
{
plan skip_all =>
'Due to execution costs these tests are skipped unless subscription
is enabled in PG_TEST_EXTRA';
}

# Add tests here...

To help strengthen the above proposal, here are a couple of examples
where TAP tests already use this strategy to avoid tests for various
reasons.

[1]: https://github.com/postgres/postgres/blob/master/src/test/recovery/t/027_stream_regress.pl
# WAL consistency checking is resource intensive so require opt-in with the
# PG_TEST_EXTRA environment variable.
if ( $ENV{PG_TEST_EXTRA}
&& $ENV{PG_TEST_EXTRA} =~ m/\bwal_consistency_checking\b/)
{
$node_primary->append_conf('postgresql.conf',
'wal_consistency_checking = all');
}

[2]: https://github.com/postgres/postgres/blob/master/src/interfaces/libpq/t/004_load_balance_dns.pl
if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
{
plan skip_all =>
'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
}

======
[1]: https://github.com/postgres/postgres/blob/master/src/test/recovery/t/027_stream_regress.pl
[2]: https://github.com/postgres/postgres/blob/master/src/interfaces/libpq/t/004_load_balance_dns.pl

Kind Regards,
Peter Smith.
Fujitsu Australia

Tomas Vondra

tomas@vondra.me

about 1 year ago

In reply to: Peter Smith (#7)

Re: CREATE SUBSCRIPTION - add missing test case

On 8/22/24 05:21, Peter Smith wrote:

...

I also don't see a test for this error condition. However, it is not
clear to me how important is it to cover this error code path. This
code has existed for a long time and I didn't notice any bugs related
to this. There is a possibility that in the future we might break
something because of a lack of this test but not sure if we want to
cover every code path via tests as each additional test also has some
cost. OTOH, If others think it is important or a good idea to have
this test then I don't have any objection to the same.

Yes, AFAIK there were no bugs related to this; The test was proposed
to prevent accidental future bugs.

Not sure if absence of prior bug reports is a good data point to decide
which tests are useful. It seems worth checking we keep reporting the
error, even if it seems unlikely we'd break that.

BACKGROUND

Another pending feature thread (replication of generated columns) [1]
required many test combinations to confirm all the different expected
results which are otherwise easily accidentally broken without
noticing. This *current* thread test shares one of the same error
messages, which is how it was discovered missing in the first place.

~~~

Right.

PROPOSAL

I think this is not the first time a logical replication test has been
questioned due mostly to concern about creeping "costs".

How about we create a new test file and put test cases like this one
into it, guarded by code like the below using PG_TEST_EXTRA [2]?

Doing it this way we can have better code coverage and higher
confidence when we want it, but zero test cost overheads when we don't
want it.

e.g.

src/test/subscription/t/101_extra.pl:

if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bsubscription\b/)
{
plan skip_all =>
'Due to execution costs these tests are skipped unless subscription
is enabled in PG_TEST_EXTRA';
}

# Add tests here...

To help strengthen the above proposal, here are a couple of examples
where TAP tests already use this strategy to avoid tests for various
reasons.

[1] Avoids some test because of cost
# WAL consistency checking is resource intensive so require opt-in with the
# PG_TEST_EXTRA environment variable.
if ( $ENV{PG_TEST_EXTRA}
&& $ENV{PG_TEST_EXTRA} =~ m/\bwal_consistency_checking\b/)
{
$node_primary->append_conf('postgresql.conf',
'wal_consistency_checking = all');
}

[2] Avoids some tests because of safety
if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
{
plan skip_all =>
'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
}

Yes, there are cases with logical replication where reproducing may be
expensive (in terms of data amounts, time, ...) but I don't think that's
the case here - this test is trivial/cheap.

But I believe the "costs" mentioned by Amit are more about having to
maintain the tests etc. rather than execution costs. In which case
having a flag does exactly nothing - we'd still have to maintain it.

I propose we simply add the test to 008_diff_schema.pl, per v2. I see no
reason to invent something more here.

regards

--
Tomas Vondra

Peter Smith

smithpb2250@gmail.com

about 1 year ago

In reply to: Tomas Vondra (#8)

Re: CREATE SUBSCRIPTION - add missing test case

On Sun, Dec 8, 2024 at 10:57 AM Tomas Vondra <tomas@vondra.me> wrote:

On 8/22/24 05:21, Peter Smith wrote:

...

I also don't see a test for this error condition. However, it is not
clear to me how important is it to cover this error code path. This
code has existed for a long time and I didn't notice any bugs related
to this. There is a possibility that in the future we might break
something because of a lack of this test but not sure if we want to
cover every code path via tests as each additional test also has some
cost. OTOH, If others think it is important or a good idea to have
this test then I don't have any objection to the same.

Yes, AFAIK there were no bugs related to this; The test was proposed
to prevent accidental future bugs.

Not sure if absence of prior bug reports is a good data point to decide
which tests are useful. It seems worth checking we keep reporting the
error, even if it seems unlikely we'd break that.

BACKGROUND

Another pending feature thread (replication of generated columns) [1]
required many test combinations to confirm all the different expected
results which are otherwise easily accidentally broken without
noticing. This *current* thread test shares one of the same error
messages, which is how it was discovered missing in the first place.

~~~

Right.

PROPOSAL

I think this is not the first time a logical replication test has been
questioned due mostly to concern about creeping "costs".

How about we create a new test file and put test cases like this one
into it, guarded by code like the below using PG_TEST_EXTRA [2]?

Doing it this way we can have better code coverage and higher
confidence when we want it, but zero test cost overheads when we don't
want it.

e.g.

src/test/subscription/t/101_extra.pl:

if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bsubscription\b/)
{
plan skip_all =>
'Due to execution costs these tests are skipped unless subscription
is enabled in PG_TEST_EXTRA';
}

# Add tests here...

To help strengthen the above proposal, here are a couple of examples
where TAP tests already use this strategy to avoid tests for various
reasons.

[1] Avoids some test because of cost
# WAL consistency checking is resource intensive so require opt-in with the
# PG_TEST_EXTRA environment variable.
if ( $ENV{PG_TEST_EXTRA}
&& $ENV{PG_TEST_EXTRA} =~ m/\bwal_consistency_checking\b/)
{
$node_primary->append_conf('postgresql.conf',
'wal_consistency_checking = all');
}

[2] Avoids some tests because of safety
if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
{
plan skip_all =>
'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
}

Yes, there are cases with logical replication where reproducing may be
expensive (in terms of data amounts, time, ...) but I don't think that's
the case here - this test is trivial/cheap.

But I believe the "costs" mentioned by Amit are more about having to
maintain the tests etc. rather than execution costs. In which case
having a flag does exactly nothing - we'd still have to maintain it.

I propose we simply add the test to 008_diff_schema.pl, per v2. I see no
reason to invent something more here.

Hi Tomas,

Since you think patch v2 is already OK as-is, I have changed the
commitfest entry [1]https://commitfest.postgresql.org/51/5190/ for this to RfC.

======
[1]: https://commitfest.postgresql.org/51/5190/

Kind Regards,
Peter Smith.
Fujitsu Australia

#10

Heikki Linnakangas

hlinnaka@iki.fi

10 months ago

In reply to: Peter Smith (#9)

Re: CREATE SUBSCRIPTION - add missing test case

On 10/01/2025 05:11, Peter Smith wrote:

On Sun, Dec 8, 2024 at 10:57 AM Tomas Vondra <tomas@vondra.me> wrote:

Yes, there are cases with logical replication where reproducing may be
expensive (in terms of data amounts, time, ...) but I don't think that's
the case here - this test is trivial/cheap.

But I believe the "costs" mentioned by Amit are more about having to
maintain the tests etc. rather than execution costs. In which case
having a flag does exactly nothing - we'd still have to maintain it.

I propose we simply add the test to 008_diff_schema.pl, per v2. I see no
reason to invent something more here.

Hi Tomas,

Since you think patch v2 is already OK as-is, I have changed the
commitfest entry [1] for this to RfC.

Committed, thanks!

It's always a balancing act between test coverage and how long the tests
run. I agree this test is worth it, albeit with a thin margin.

--
Heikki Linnakangas
Neon (https://neon.tech)

#11

Peter Smith

smithpb2250@gmail.com

10 months ago

In reply to: Heikki Linnakangas (#10)

Re: CREATE SUBSCRIPTION - add missing test case

Thanks for pushing.

======
Kind Regards,
Peter Smith.
Fujitsu Australia