Re: [PATCH TEST] Fix logical replication setup in subscription test `t/009_matviews.pl`

On Thu, Oct 16, 2025 at 3:45 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Oct 14, 2025 at 10:34 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >
> > On Mon, Oct 13, 2025 at 06:12:07PM +0530, Ashutosh Bapat wrote:
> > > For now this makes sense.
> >
> > The arguments and the patches I am seeing do not really make sense
> > here.
> >
> > > We could avoid running a full test, and save time and resources, if we
> > > piggy back MV vs logical replication testing on 100_bugs.pl. If we do
> > > that, it should be master-only and can be a separate discussion.
> >
> > Moving the test would be one thing, I'd be OK to do that only on HEAD
> > and not bother about the back-branches. Still, that's a separate
> > discussion.
> >
> > As far as I can see, this test is not able to check what it wants to
> > check since v17. Hence, doing something only on HEAD, if that's
> > really what you are implying here (I understand that you are not
> > implying that but I am not entirely sure, either), is not sufficient.
> >
> > I also fail to understand how Kuroda-san's patch helps in solving
> > everything at hand: more is required. If apply the patch posted at
> > [1] on HEAD, the test passes. If I revert the change introduced by
> > bc1adc651b8e in pgoutput.c with the patch posted at [1], the test
> > passes as well, but I would expect that the test *fails* in some way.
> >
> > I did not take the time to dive into the details here, but should
> > there be at least some pattern detected in the TAP test? Or do we
> > actually need to patch pgoutput.c at all now and remove the check
> > based on is_publishable_relation(). Or is there any meaning in
> > spending cycles for such a test now if we cannot detect a difference
> > in behavior?
> >
>
> After commit b4da732fd64e, the is_publishable_relation() in
> pgoutput_change() is not required for materialized views but the check
> in itself is a good backstop to prevent any non-publishable change to
> be sent to the client. Now, there is an argument that the
> is_publishable_relation() check can be removed from pgoutput_change()
> but I think it is a matter of separate research as the code coverage
> report shows it is covered by some regression test [1]. Even if it is
> not covered by any existing test, I feel this is a harmless check and
> probably needs more study if we want to remove it or add some
> elog(LOG, ...).
>
> The patch proposed in the first email in this thread is a simple
> change to improve the existing test and can be considered to commit
> irrespective of what we decide for the is_publishable_relation() check
> in pgoutput_change().
>

I am planning to push the initial patch proposed in this thread early
next week unless someone thinks otherwise.

--
With Regards,
Amit Kapila.