On Tue, Jul 5, 2022 at 1:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Jul 4, 2022 at 11:33 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Mon, Jul 4, 2022 at 7:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > >
> > > > From d8f8844f877806527b6f3f45320b6ba55a8e3154 Mon Sep 17 00:00:00 2001
> > > > From: Vigneshwaran C <vignesh21@gmail.com>
> > > > Date: Thu, 26 May 2022 19:29:33 +0530
> > > > Subject: [PATCH v27 1/4] Add a missing test to verify only-local parameter in
> > > > test_decoding plugin.
> > > >
> > > > Add a missing test to verify only-local parameter in test_decoding plugin.
> > >
> > > I don't get it. replorigin.sql already has some lines to test
> > > local-only. What is your patch adding that is new? Maybe instead of
> > > adding some more lines at the end of the script, you should add lines
> > > where this stuff is already being tested. But that assumes that there
> > > is something new that is being tested; if so what is it?
> >
> > The test is to check that remote origin data (i.e. replication origin
> > being set) will be filtered when only-local parameter is set. I felt
> > that this scenario is not covered, unless I'm missing something.
> >
>
> If we change only-local option to '0' in the below part of the test,
> we can see a different result:
> -- and magically the replayed xact will be filtered!
> SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL,
> NULL, 'include-xids', '0', 'skip-empty-xacts', '1', 'only-local',
> '1');
>
> So, I think only-local functionality is being tested but I feel your
> test is clear in the sense that it clearly shows that remote data can
> only be fetched with 'only-local' as '0' when the origin is set. It
> seems to me that in existing tests, we are not testing the combination
> where the origin is set and 'only-local' is '0'. So, there is some
> value to having the test you are proposing but if Alvaro, you, or
> others don't think it is worth adding a new test for it then we can
> probably drop this one.
Since the existing test is already handling the verification of this
scenario, I felt no need to add the test. Updated v29 patch removes
the 0001 patch which had the test case.
Regards,
Vignesh