On Wed, Jul 10, 2019 at 12:47 PM Tom Lane <t...@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
0001-Warn-if-wal_level-is-too-low-when-creating-a-publica.patch
Description: Binary data