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

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().

[1] - https://coverage.postgresql.org/src/backend/replication/pgoutput/pgoutput.c.gcov.html

--
With Regards,
Amit Kapila.