Re: CREATE SUBSCRIPTION - add missing test case

On Wed, Aug 21, 2024 at 8:48 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Aug 16, 2024 at 9:45 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Thu, 15 Aug 2024 at 12:55, Peter Smith <smithpb2250(at)gmail(dot)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...

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

Kind Regards,
Peter Smith.
Fujitsu Australia