Thread: warning to publication created and wal_level is not set to logical

On Thu, Mar 21, 2019 at 07:45:59PM -0300, Lucas Viecelli wrote:
> Hi everyone,
> 
> A very common question among new users is how wal_level works and it
> levels. I heard about some situations like that, a user create a new
> publication in its master database and he/she simply does not change
> wal_level to logical, sometimes, this person lost maintenance
> window, or a chance to restart postgres service, usually a
> production database, and it will discover that wal_level is not
> right just in subscription creation.  Attempting to iterate between
> new (and even experienced) users with logical replication, I am
> sending a patch that when an PUBLICATION is created and the
> wal_level is different from logical prints a WARNING in console/log:

Is a WARNING sufficient?  Maybe I'm misunderstanding something
important, but I think the attempt should fail with a HINT to set the
wal_level ahead of time.

Possibly in a separate patch, setting the wal_level to anything lower
than logical when publications exist should also fail.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david@fetter.org> writes:
> On Thu, Mar 21, 2019 at 07:45:59PM -0300, Lucas Viecelli wrote:
>> I am sending a patch that when an PUBLICATION is created and the
>> wal_level is different from logical prints a WARNING in console/log:

> Is a WARNING sufficient?  Maybe I'm misunderstanding something
> important, but I think the attempt should fail with a HINT to set the
> wal_level ahead of time.

That would be a booby-trap for dump/restore and pg_upgrade, so I don't
think making CREATE PUBLICATION fail outright would be wise.

> Possibly in a separate patch, setting the wal_level to anything lower
> than logical when publications exist should also fail.

I do not believe this is practical either.  GUC manipulation cannot
look at the catalogs.

I agree that it'd be nice to be noisier about the problem, but I'm
not sure we can do more than bleat in the postmaster log from time
to time if a publication is active and wal_level is too low.
(And we'd better be careful about the log-spam aspect of that...)

            regards, tom lane

On Sun, Mar 24, 2019 at 02:06:59PM -0400, Tom Lane wrote:
> David Fetter <david@fetter.org> writes:
> > On Thu, Mar 21, 2019 at 07:45:59PM -0300, Lucas Viecelli wrote:
> >> I am sending a patch that when an PUBLICATION is created and the
> >> wal_level is different from logical prints a WARNING in console/log:
> 
> > Is a WARNING sufficient?  Maybe I'm misunderstanding something
> > important, but I think the attempt should fail with a HINT to set the
> > wal_level ahead of time.
> 
> That would be a booby-trap for dump/restore and pg_upgrade, so I don't
> think making CREATE PUBLICATION fail outright would be wise.

I haven't yet come up with a situation where it would be appropriate
both for wal_level to be below logical and for a PUBLICATION to exist,
even as some intermediate state during pg_restore.

> > Possibly in a separate patch, setting the wal_level to anything lower
> > than logical when publications exist should also fail.
> 
> I do not believe this is practical either.  GUC manipulation cannot
> look at the catalogs.

In this case, it really has to do something. Is setting GUCs a path so
critical it can't take one branch?

> I agree that it'd be nice to be noisier about the problem, but I'm
> not sure we can do more than bleat in the postmaster log from time
> to time if a publication is active and wal_level is too low.
> (And we'd better be careful about the log-spam aspect of that...)

With utmost respect, we have a lot more responsibility to the users of
this feature than this might imply. If there are circumstances where
there should be both a PUBLICATION and a wal_level less than logical,
by all means, let's document them very clearly in all the relevant
places.

If, as I strongly suspect, no such circumstance exists, it should not
be possible for someone to have both of those at once, however
inconvenient it is for us to arrange it.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Mon, Mar 25, 2019 at 10:15 AM David Fetter <david@fetter.org> wrote:
> > I do not believe this is practical either.  GUC manipulation cannot
> > look at the catalogs.
>
> In this case, it really has to do something. Is setting GUCs a path so
> critical it can't take one branch?

No, but that has about zero to do with the actual problem that Tom is
describing.

> If, as I strongly suspect, no such circumstance exists, it should not
> be possible for someone to have both of those at once, however
> inconvenient it is for us to arrange it.

Uh, Tom already told you how it can happen.  You just take a pg_dump
of an existing database, run initdb to create a new cluster, and then
try to restore the dump on the new cluster.  That shouldn't fail just
because wal_level = 'logical' isn't configured yet.  If it did, that
would be creating a huge booby-trap for users that doesn't exist
today.  You can't just dismiss that as nothing.  I think users have
every right to expect that a dump and restore is going to work without
preconfiguring things like wal_level -- it's bad enough that you
already have to struggle with things like encoding to get dumps to
restore properly.  Adding more ways for dump restoration to fail is a
really bad idea.

Besides that, it is obviously impractical to stop somebody from
shutting down the server, changing wal_level, and then restarting the
server.  Nor can you make all publications magically go away if
someone does that.  Nor would it be a good idea if we could do that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Mar 25, 2019 at 10:15 AM David Fetter <david@fetter.org> wrote:
>>> I do not believe this is practical either.  GUC manipulation cannot
>>> look at the catalogs.

>> In this case, it really has to do something. Is setting GUCs a path so
>> critical it can't take one branch?

> No, but that has about zero to do with the actual problem that Tom is
> describing.

To clarify, the problems with that are

(1) Initial GUC settings are absorbed by the postmaster, which cannot
examine catalogs *at all*.  It is neither connected to any database
nor allowed to participate in transactions.  These are not things that
will change.

(2) wal_level is a global setting, but the catalogs we'd have to look
at to discover the existence of a publication are per-database.  Thus
for example there is no reliable way for "ALTER SYSTEM SET wal_level"
to detect whether publications exist in other databases of the cluster.
(To say nothing of race conditions against concurrent publication
creation commands.)

Adding the dump/restore issue on top of that, it seems clear to me that
we can't usefully prevent a conflicting setting of wal_level from being
established.  The best we can do is whine about it later.

One idea that might be useful is to have walsenders refuse to transmit
any logical-replication data if they see wal_level is too low.  That
would get users' attention pretty quickly.

            regards, tom lane

Hi,

On 2019-03-25 13:53:32 -0400, Tom Lane wrote:
> One idea that might be useful is to have walsenders refuse to transmit
> any logical-replication data if they see wal_level is too low.  That
> would get users' attention pretty quickly.

They do:


/*
 * Load previously initiated logical slot and prepare for sending data (via
 * WalSndLoop).
 */
static void
StartLogicalReplication(StartReplicationCmd *cmd)
{
    StringInfoData buf;

    /* make sure that our requirements are still fulfilled */
    CheckLogicalDecodingRequirements();

and CheckLogicalDecodingReqs contains:

    if (wal_level < WAL_LEVEL_LOGICAL)
        ereport(ERROR,
                (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                 errmsg("logical decoding requires wal_level >= logical")));


Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2019-03-25 13:53:32 -0400, Tom Lane wrote:
>> One idea that might be useful is to have walsenders refuse to transmit
>> any logical-replication data if they see wal_level is too low.  That
>> would get users' attention pretty quickly.

> They do:

Oh, OK, then this seems like it's basically covered already.  I think
the original suggestion to add a WARNING during CREATE PUBLICATION
isn't unreasonable.  But we don't need to do more than that (and it
shouldn't be higher than WARNING).

            regards, tom lane

On Wed, Mar 27, 2019 at 1:36 AM Lucas Viecelli <lviecelli199@gmail.com> wrote:
>> Oh, OK, then this seems like it's basically covered already.  I think
>> the original suggestion to add a WARNING during CREATE PUBLICATION
>> isn't unreasonable.  But we don't need to do more than that (and it
>> shouldn't be higher than WARNING).
>
> Okay, I think it will improve understanding of new users.
>
> Since everything is fine, thank you all for the comments

Hi Lucas,

The July Commitfest has started.  This patch is in "Needs review"
status, but it doesn't apply.  If I read the above discussion
correctly, it seems there is agreement that a warning here is a good
idea to commit this patch.  Could you please post a rebased patch?

A note on the message:

WARNING: `PUBLICATION` created but wal_level `is` not set to logical,
you need to change it before creating any SUBSCRIPTION

I wonder if it would be more typical project style to put the clue on
what to do into an "errhint" message, something like this:

WARNING: insufficient wal_level to publish logical changes
HINT:  Set wal_level to logical before creating subscriptions.

-- 
Thomas Munro
https://enterprisedb.com

On Tue, Jul 9, 2019 at 5:40 PM Lucas Viecelli <lviecelli199@gmail.com> wrote:
> Follow the correct file, I added the wrong patch in the previous email

New status: Ready for Committer.  If nobody wants to bikeshed the
wording or other details, I will commit this tomorrow.

-- 
Thomas Munro
https://enterprisedb.com

Thomas Munro <thomas.munro@gmail.com> writes:
> New status: Ready for Committer.  If nobody wants to bikeshed the
> wording or other details, I will commit this tomorrow.

Hm, so:

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.

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.

            regards, tom lane

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

Thomas Munro <thomas.munro@gmail.com> writes:
> On Wed, Jul 10, 2019 at 12:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.

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

WFM.

            regards, tom lane

On Sat, Jul 13, 2019 at 3:21 AM Lucas Viecelli <lviecelli199@gmail.com> wrote:
>> Agreed, fixed.  Also run through pgindent
>
> Thank you for the adjustments.

> All right, for me. If wal_level can not interfere with the testes result, it seems to a better approach

Pushed.  Thanks for the patch!

-- 
Thomas Munro
https://enterprisedb.com