On Wed, Jul 10, 2019 at 12:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 1.
>
> + errmsg("insufficient wal_level to publish logical changes"),
>
> Might read better as "wal_level is insufficient to publish logical changes"?
>
> 2.
>
> + errhint("Set wal_level to logical before creating subscriptions")));
>
> This definitely is not per style guidelines, needs a trailing period.
Agreed, fixed. Also run through pgindent.
> 3. AFAICS, the proposed test case changes will cause the core regression
> tests to fail if wal_level is not replica. This is not true today ---
> they pass regardless of wal_level --- and I object in the strongest terms
> to making it otherwise.
>
> I'm not really convinced that we need regression tests for this change at
> all, but if we do, put them in one of the TAP replication test suites,
> which already depend on wal_level being set to something in particular.
I agree that it's not really worth having tests for this, and I take
your point about the dependency on wal_level that we don't currently
have. The problem is that the core tests include publications
already, and it doesn't seem like a great idea to move the whole lot
to a TAP test. Creating alternative expected files seems like a bad
idea too (annoying to maintain, wouldn't compose well with the next
thing like this). So... how about we just suppress WARNINGs for
CREATE PUBLICATION commands that are expected to succeed? Like in the
attached. This version passes installcheck with any wal_level.
--
Thomas Munro
https://enterprisedb.com