Horton

Thread

Commits

  1. Warn if wal_level is too low when creating a publication.

  1. warning to publication created and wal_level is not set to logical

    Lucas Viecelli <lviecelli199@gmail.com> — 2019-03-21T22:45:59Z

    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:

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

Initiatives like this can make a good user experience with PostgreSQL and
its own logical replication.

Thanks

--

*Lucas Viecelli*

<http://www.leosoft.com.br/coopcred>

  2. Re: warning to publication created and wal_level is not set to logical

    David Fetter <david@fetter.org> — 2019-03-24T17:54:48Z

    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

  3. Re: warning to publication created and wal_level is not set to logical

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-24T18:06:59Z

    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

  4. Re: warning to publication created and wal_level is not set to logical

    Lucas Viecelli <lviecelli199@gmail.com> — 2019-03-25T13:20:54Z

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

I thought about this possibility, but I was afraid to change the current
behavior a lot, but it's worth discussing.


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

I agree on being noisier, but I think the main thing is to let the user
aware of the situation and in that the
patch resolves, stating that he needs to adjust wal_level. Initially
WARNING will appear only at the time
the publication is created, precisely not to put spam in the log.

Is it better to warn from time to time that wal_level needs to change
because it has some publication that will not work?
-- 

*Lucas Viecelli*


<http://www.leosoft.com.br/coopcred>

  5. Re: warning to publication created and wal_level is not set to logical

    David Fetter <david@fetter.org> — 2019-03-25T14:15:33Z

    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

  6. Re: warning to publication created and wal_level is not set to logical

    Robert Haas <robertmhaas@gmail.com> — 2019-03-25T17:39:27Z

    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

  7. Re: warning to publication created and wal_level is not set to logical

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-25T17:53:32Z

    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

  8. Re: warning to publication created and wal_level is not set to logical

    Andres Freund <andres@anarazel.de> — 2019-03-25T18:06:13Z

    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

  9. Re: warning to publication created and wal_level is not set to logical

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-25T18:19:56Z

    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

  10. Re: warning to publication created and wal_level is not set to logical

    Lucas Viecelli <lviecelli199@gmail.com> — 2019-03-26T12:36:10Z

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

I checked this before creating the patch


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

Atenciosamente.

*Lucas Viecelli*

<http://www.leosoft.com.br/coopcred>

  11. Re: warning to publication created and wal_level is not set to logical

    Thomas Munro <thomas.munro@gmail.com> — 2019-07-08T03:04:34Z

    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

  12. Re: warning to publication created and wal_level is not set to logical

    Lucas Viecelli <lviecelli199@gmail.com> — 2019-07-09T03:56:51Z

    Hi Thomas.

Attached is the rebased


> 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?
>
>
I followed your suggestion and changed the message and added HINT. I hope
everything is agreed now.


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

-- 

*Lucas Viecelli*


<http://www.leosoft.com.br/coopcred>

  13. Re: warning to publication created and wal_level is not set to logical

    Lucas Viecelli <lviecelli199@gmail.com> — 2019-07-09T05:40:42Z

    Follow the correct file, I added the wrong patch in the previous email


> Attached is the rebased
>


thanks a lot

*Lucas Viecelli*

  14. Re: warning to publication created and wal_level is not set to logical

    Thomas Munro <thomas.munro@gmail.com> — 2019-07-09T23:43:40Z

    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

  15. Re: warning to publication created and wal_level is not set to logical

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-07-10T00:47:08Z

    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

  16. Re: warning to publication created and wal_level is not set to logical

    Thomas Munro <thomas.munro@gmail.com> — 2019-07-11T05:14:31Z

    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

  17. Re: warning to publication created and wal_level is not set to logical

    Lucas Viecelli <lviecelli199@gmail.com> — 2019-07-12T15:21:33Z

    > Agreed, fixed.  Also run through pgindent
>

Thank you for the adjustments.


> 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.
>
All right, for me. If wal_level can not interfere with the testes result,
it seems to a better approach

*Lucas Viecelli*


<http://www.leosoft.com.br/coopcred>

  18. Re: warning to publication created and wal_level is not set to logical

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-07-12T15:33:35Z

    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

  19. Re: warning to publication created and wal_level is not set to logical

    Thomas Munro <thomas.munro@gmail.com> — 2019-07-13T01:10:32Z

    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