Horton

Thread

Commits

  1. Revert "Improve handling of parameter differences in physical replication"

  2. Improve handling of parameter differences in physical replication

  1. pgsql: Improve handling of parameter differences in physical replicatio

    Peter Eisentraut <peter@eisentraut.org> — 2020-03-30T07:58:54Z

    Improve handling of parameter differences in physical replication

When certain parameters are changed on a physical replication primary,
this is communicated to standbys using the XLOG_PARAMETER_CHANGE WAL
record.  The standby then checks whether its own settings are at least
as big as the ones on the primary.  If not, the standby shuts down
with a fatal error.

The correspondence of settings between primary and standby is required
because those settings influence certain shared memory sizings that
are required for processing WAL records that the primary might send.
For example, if the primary sends a prepared transaction, the standby
must have had max_prepared_transaction set appropriately or it won't
be able to process those WAL records.

However, fatally shutting down the standby immediately upon receipt of
the parameter change record might be a bit of an overreaction.  The
resources related to those settings are not required immediately at
that point, and might never be required if the activity on the primary
does not exhaust all those resources.  If we just let the standby roll
on with recovery, it will eventually produce an appropriate error when
those resources are used.

So this patch relaxes this a bit.  Upon receipt of
XLOG_PARAMETER_CHANGE, we still check the settings but only issue a
warning and set a global flag if there is a problem.  Then when we
actually hit the resource issue and the flag was set, we issue another
warning message with relevant information.  At that point we pause
recovery, so a hot standby remains usable.  We also repeat the last
warning message once a minute so it is harder to miss or ignore.

Reviewed-by: Sergei Kornilov <sk@zsrv.org>
Reviewed-by: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/4ad69a4c-cc9b-0dfe-0352-8b1b0cd36c7b@2ndquadrant.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/246f136e76ecd26844840f2b2057e2c87ec9868d

Modified Files
--------------
doc/src/sgml/high-availability.sgml   | 48 +++++++++++++++++------
src/backend/access/transam/twophase.c |  3 ++
src/backend/access/transam/xlog.c     | 74 ++++++++++++++++++++++++++++++-----
src/backend/storage/ipc/procarray.c   |  9 ++++-
src/backend/storage/lmgr/lock.c       | 10 +++++
src/include/access/xlog.h             |  1 +
6 files changed, 122 insertions(+), 23 deletions(-)

  2. Re: pgsql: Improve handling of parameter differences in physical replicatio

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-03-30T10:41:43Z

    
On 2020/03/30 16:58, Peter Eisentraut wrote:
> Improve handling of parameter differences in physical replication
> 
> When certain parameters are changed on a physical replication primary,
> this is communicated to standbys using the XLOG_PARAMETER_CHANGE WAL
> record.  The standby then checks whether its own settings are at least
> as big as the ones on the primary.  If not, the standby shuts down
> with a fatal error.
> 
> The correspondence of settings between primary and standby is required
> because those settings influence certain shared memory sizings that
> are required for processing WAL records that the primary might send.
> For example, if the primary sends a prepared transaction, the standby
> must have had max_prepared_transaction set appropriately or it won't
> be able to process those WAL records.
> 
> However, fatally shutting down the standby immediately upon receipt of
> the parameter change record might be a bit of an overreaction.  The
> resources related to those settings are not required immediately at
> that point, and might never be required if the activity on the primary
> does not exhaust all those resources.  If we just let the standby roll
> on with recovery, it will eventually produce an appropriate error when
> those resources are used.
> 
> So this patch relaxes this a bit.  Upon receipt of
> XLOG_PARAMETER_CHANGE, we still check the settings but only issue a
> warning and set a global flag if there is a problem.  Then when we
> actually hit the resource issue and the flag was set, we issue another
> warning message with relevant information.  At that point we pause
> recovery, so a hot standby remains usable.  We also repeat the last
> warning message once a minute so it is harder to miss or ignore.

I encountered the trouble maybe related to this commit.

Firstly I set up the master and the standby with max_connections=100 (default value).
Then I decreased max_connections to 1 only in the standby and restarted
the server. Thanks to the commit, I saw the following warning message
in the standby.

     WARNING:  insufficient setting for parameter max_connections
     DETAIL:  max_connections = 1 is a lower setting than on the master server (where its value was 100).
     HINT:  Change parameters and restart the server, or there may be resource exhaustion errors sooner or later.

Then I made the script that inserted 1,000,000 rows in one transaction,
and ran it 30 times at the same time. That is, 30 transactions inserting
lots of rows were running at the same time.

I confirmed that there are expected number of rows in the master,
but found 0 row in the standby unxpectedly. Also I suspected that issue
happened because recovery is paused, but pg_is_wal_replay_paused()
returned false in the standby.

Isn't this the trouble related to this commit?

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

  3. Re: pgsql: Improve handling of parameter differences in physical replicatio

    Andres Freund <andres@anarazel.de> — 2020-03-30T18:10:30Z

    Hi,

On 2020-03-30 19:41:43 +0900, Fujii Masao wrote:
> On 2020/03/30 16:58, Peter Eisentraut wrote:
> > Improve handling of parameter differences in physical replication
> > 
> > When certain parameters are changed on a physical replication primary,
> > this is communicated to standbys using the XLOG_PARAMETER_CHANGE WAL
> > record.  The standby then checks whether its own settings are at least
> > as big as the ones on the primary.  If not, the standby shuts down
> > with a fatal error.
> > 
> > The correspondence of settings between primary and standby is required
> > because those settings influence certain shared memory sizings that
> > are required for processing WAL records that the primary might send.
> > For example, if the primary sends a prepared transaction, the standby
> > must have had max_prepared_transaction set appropriately or it won't
> > be able to process those WAL records.
> > 
> > However, fatally shutting down the standby immediately upon receipt of
> > the parameter change record might be a bit of an overreaction.  The
> > resources related to those settings are not required immediately at
> > that point, and might never be required if the activity on the primary
> > does not exhaust all those resources.  If we just let the standby roll
> > on with recovery, it will eventually produce an appropriate error when
> > those resources are used.
> > 
> > So this patch relaxes this a bit.  Upon receipt of
> > XLOG_PARAMETER_CHANGE, we still check the settings but only issue a
> > warning and set a global flag if there is a problem.  Then when we
> > actually hit the resource issue and the flag was set, we issue another
> > warning message with relevant information.

I find it somewhat hostile that we don't display the actual resource
error once the problem is hit - we just pause. Sure, there's going to be
some previous log entry explaining what the actual parameter difference
is - but that could have been weeks ago. So either hard to find, or even
rotated out.


> > At that point we pause recovery, so a hot standby remains usable.
> > We also repeat the last warning message once a minute so it is
> > harder to miss or ignore.


I can't really imagine that the adjustments made in this patch are
sufficient.

One important issue seems to me to be the size of the array that
TransactionIdIsInProgress() allocates:
	/*
	 * If first time through, get workspace to remember main XIDs in. We
	 * malloc it permanently to avoid repeated palloc/pfree overhead.
	 */
	if (xids == NULL)
	{
		/*
		 * In hot standby mode, reserve enough space to hold all xids in the
		 * known-assigned list. If we later finish recovery, we no longer need
		 * the bigger array, but we don't bother to shrink it.
		 */
		int			maxxids = RecoveryInProgress() ? TOTAL_MAX_CACHED_SUBXIDS : arrayP->maxProcs;

		xids = (TransactionId *) malloc(maxxids * sizeof(TransactionId));
		if (xids == NULL)
			ereport(ERROR,
					(errcode(ERRCODE_OUT_OF_MEMORY),
					 errmsg("out of memory")));
	}

Which I think means we'll just overrun the xids array in some cases,
e.g. if KnownAssignedXids overflowed.  Obviously we should have a
crosscheck in the code (which we don't), but it was previously a
supposedly unreachable path.

Similarly, the allocation in GetSnapshotData() will be too small, I
think:
	if (snapshot->xip == NULL)
	{
		/*
		 * First call for this snapshot. Snapshot is same size whether or not
		 * we are in recovery, see later comments.
		 */
		snapshot->xip = (TransactionId *)
			malloc(GetMaxSnapshotXidCount() * sizeof(TransactionId));
		if (snapshot->xip == NULL)
			ereport(ERROR,
					(errcode(ERRCODE_OUT_OF_MEMORY),
					 errmsg("out of memory")));
		Assert(snapshot->subxip == NULL);
		snapshot->subxip = (TransactionId *)
			malloc(GetMaxSnapshotSubxidCount() * sizeof(TransactionId));
		if (snapshot->subxip == NULL)
			ereport(ERROR,
					(errcode(ERRCODE_OUT_OF_MEMORY),
					 errmsg("out of memory")));
	}

I think basically all code using TOTAL_MAX_CACHED_SUBXIDS,
GetMaxSnapshotSubxidCount(), PROCARRAY_MAXPROCS needs to be reviewed
much more carefully than done here.


Also, shouldn't dynahash be adjusted as well? There's e.g. the
following HASH_ENTER path:
				/* report a generic message */
				if (hashp->isshared)
					ereport(ERROR,
							(errcode(ERRCODE_OUT_OF_MEMORY),
							 errmsg("out of shared memory")));


I'm also not sure it's ok to not have the waiting in
ProcArrayAdd(). Is it guaranteed that can't be hit now, due to the WAL
replay path sometimes adding procs?


How is it safe to just block in StandbyParamErrorPauseRecovery() before
raising an error? The error would have released lwlocks, but now we
don't? If we e.g. block in PrepareRedoAdd() we'll continue to hold
TwoPhaseStateLock(), but shutting the database down might also acquire
that (CheckPointTwoPhase()). Similar with the error in
KnownAssignedXidsAdd().


This does not seem ready.


> I encountered the trouble maybe related to this commit.
> 
> Firstly I set up the master and the standby with max_connections=100 (default value).
> Then I decreased max_connections to 1 only in the standby and restarted
> the server. Thanks to the commit, I saw the following warning message
> in the standby.
> 
>     WARNING:  insufficient setting for parameter max_connections
>     DETAIL:  max_connections = 1 is a lower setting than on the master server (where its value was 100).
>     HINT:  Change parameters and restart the server, or there may be resource exhaustion errors sooner or later.
> 
> Then I made the script that inserted 1,000,000 rows in one transaction,
> and ran it 30 times at the same time. That is, 30 transactions inserting
> lots of rows were running at the same time.
> 
> I confirmed that there are expected number of rows in the master,
> but found 0 row in the standby unxpectedly.

Have the relevant records actually been replicated?


> Also I suspected that issue
> happened because recovery is paused, but pg_is_wal_replay_paused()
> returned false in the standby.

As far as I understand the commit it shouldn't cause a pause until
there's an actual resource exhaustion - in which case there should be
another message first (WARNING: recovery paused because of insufficient
parameter settings).


Greetings,

Andres Freund

  4. Re: pgsql: Improve handling of parameter differences in physical replicatio

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-03-31T01:16:03Z

    
On 2020/03/31 3:10, Andres Freund wrote:
> Hi,
> 
> On 2020-03-30 19:41:43 +0900, Fujii Masao wrote:
>> On 2020/03/30 16:58, Peter Eisentraut wrote:
>>> Improve handling of parameter differences in physical replication
>>>
>>> When certain parameters are changed on a physical replication primary,
>>> this is communicated to standbys using the XLOG_PARAMETER_CHANGE WAL
>>> record.  The standby then checks whether its own settings are at least
>>> as big as the ones on the primary.  If not, the standby shuts down
>>> with a fatal error.
>>>
>>> The correspondence of settings between primary and standby is required
>>> because those settings influence certain shared memory sizings that
>>> are required for processing WAL records that the primary might send.
>>> For example, if the primary sends a prepared transaction, the standby
>>> must have had max_prepared_transaction set appropriately or it won't
>>> be able to process those WAL records.
>>>
>>> However, fatally shutting down the standby immediately upon receipt of
>>> the parameter change record might be a bit of an overreaction.  The
>>> resources related to those settings are not required immediately at
>>> that point, and might never be required if the activity on the primary
>>> does not exhaust all those resources.  If we just let the standby roll
>>> on with recovery, it will eventually produce an appropriate error when
>>> those resources are used.
>>>
>>> So this patch relaxes this a bit.  Upon receipt of
>>> XLOG_PARAMETER_CHANGE, we still check the settings but only issue a
>>> warning and set a global flag if there is a problem.  Then when we
>>> actually hit the resource issue and the flag was set, we issue another
>>> warning message with relevant information.
> 
> I find it somewhat hostile that we don't display the actual resource
> error once the problem is hit - we just pause. Sure, there's going to be
> some previous log entry explaining what the actual parameter difference
> is - but that could have been weeks ago. So either hard to find, or even
> rotated out.
> 
> 
>>> At that point we pause recovery, so a hot standby remains usable.
>>> We also repeat the last warning message once a minute so it is
>>> harder to miss or ignore.
> 
> 
> I can't really imagine that the adjustments made in this patch are
> sufficient.
> 
> One important issue seems to me to be the size of the array that
> TransactionIdIsInProgress() allocates:
> 	/*
> 	 * If first time through, get workspace to remember main XIDs in. We
> 	 * malloc it permanently to avoid repeated palloc/pfree overhead.
> 	 */
> 	if (xids == NULL)
> 	{
> 		/*
> 		 * In hot standby mode, reserve enough space to hold all xids in the
> 		 * known-assigned list. If we later finish recovery, we no longer need
> 		 * the bigger array, but we don't bother to shrink it.
> 		 */
> 		int			maxxids = RecoveryInProgress() ? TOTAL_MAX_CACHED_SUBXIDS : arrayP->maxProcs;
> 
> 		xids = (TransactionId *) malloc(maxxids * sizeof(TransactionId));
> 		if (xids == NULL)
> 			ereport(ERROR,
> 					(errcode(ERRCODE_OUT_OF_MEMORY),
> 					 errmsg("out of memory")));
> 	}
> 
> Which I think means we'll just overrun the xids array in some cases,
> e.g. if KnownAssignedXids overflowed.  Obviously we should have a
> crosscheck in the code (which we don't), but it was previously a
> supposedly unreachable path.
> 
> Similarly, the allocation in GetSnapshotData() will be too small, I
> think:
> 	if (snapshot->xip == NULL)
> 	{
> 		/*
> 		 * First call for this snapshot. Snapshot is same size whether or not
> 		 * we are in recovery, see later comments.
> 		 */
> 		snapshot->xip = (TransactionId *)
> 			malloc(GetMaxSnapshotXidCount() * sizeof(TransactionId));
> 		if (snapshot->xip == NULL)
> 			ereport(ERROR,
> 					(errcode(ERRCODE_OUT_OF_MEMORY),
> 					 errmsg("out of memory")));
> 		Assert(snapshot->subxip == NULL);
> 		snapshot->subxip = (TransactionId *)
> 			malloc(GetMaxSnapshotSubxidCount() * sizeof(TransactionId));
> 		if (snapshot->subxip == NULL)
> 			ereport(ERROR,
> 					(errcode(ERRCODE_OUT_OF_MEMORY),
> 					 errmsg("out of memory")));
> 	}
> 
> I think basically all code using TOTAL_MAX_CACHED_SUBXIDS,
> GetMaxSnapshotSubxidCount(), PROCARRAY_MAXPROCS needs to be reviewed
> much more carefully than done here.
> 
> 
> Also, shouldn't dynahash be adjusted as well? There's e.g. the
> following HASH_ENTER path:
> 				/* report a generic message */
> 				if (hashp->isshared)
> 					ereport(ERROR,
> 							(errcode(ERRCODE_OUT_OF_MEMORY),
> 							 errmsg("out of shared memory")));
> 
> 
> I'm also not sure it's ok to not have the waiting in
> ProcArrayAdd(). Is it guaranteed that can't be hit now, due to the WAL
> replay path sometimes adding procs?
> 
> 
> How is it safe to just block in StandbyParamErrorPauseRecovery() before
> raising an error? The error would have released lwlocks, but now we
> don't? If we e.g. block in PrepareRedoAdd() we'll continue to hold
> TwoPhaseStateLock(), but shutting the database down might also acquire
> that (CheckPointTwoPhase()). Similar with the error in
> KnownAssignedXidsAdd().
> 
> 
> This does not seem ready.
> 
> 
>> I encountered the trouble maybe related to this commit.
>>
>> Firstly I set up the master and the standby with max_connections=100 (default value).
>> Then I decreased max_connections to 1 only in the standby and restarted
>> the server. Thanks to the commit, I saw the following warning message
>> in the standby.
>>
>>      WARNING:  insufficient setting for parameter max_connections
>>      DETAIL:  max_connections = 1 is a lower setting than on the master server (where its value was 100).
>>      HINT:  Change parameters and restart the server, or there may be resource exhaustion errors sooner or later.
>>
>> Then I made the script that inserted 1,000,000 rows in one transaction,
>> and ran it 30 times at the same time. That is, 30 transactions inserting
>> lots of rows were running at the same time.
>>
>> I confirmed that there are expected number of rows in the master,
>> but found 0 row in the standby unxpectedly.
> 
> Have the relevant records actually been replicated?

Yeah, I thought I confirmed that, but when I tried to reproduce
the issue in the clean env, I failed to do that. So which means
that I did mistake... :( Sorry for noise..

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

  5. Re: pgsql: Improve handling of parameter differences in physical replicatio

    Robert Haas <robertmhaas@gmail.com> — 2020-04-03T17:55:09Z

    On Mon, Mar 30, 2020 at 2:10 PM Andres Freund <andres@anarazel.de> wrote:
> One important issue seems to me to be the size of the array that
> TransactionIdIsInProgress() allocates:
>         /*
>          * If first time through, get workspace to remember main XIDs in. We
>          * malloc it permanently to avoid repeated palloc/pfree overhead.
>          */
>         if (xids == NULL)
>         {
>                 /*
>                  * In hot standby mode, reserve enough space to hold all xids in the
>                  * known-assigned list. If we later finish recovery, we no longer need
>                  * the bigger array, but we don't bother to shrink it.
>                  */
>                 int                     maxxids = RecoveryInProgress() ? TOTAL_MAX_CACHED_SUBXIDS : arrayP->maxProcs;
>
>                 xids = (TransactionId *) malloc(maxxids * sizeof(TransactionId));
>                 if (xids == NULL)
>                         ereport(ERROR,
>                                         (errcode(ERRCODE_OUT_OF_MEMORY),
>                                          errmsg("out of memory")));
>         }
>
> Which I think means we'll just overrun the xids array in some cases,
> e.g. if KnownAssignedXids overflowed.  Obviously we should have a
> crosscheck in the code (which we don't), but it was previously a
> supposedly unreachable path.

I think this patch needs to be reverted. The only places where it
changes anything are places where we were about to throw some error
anyway. But as Andres's analysis shows, that's not nearly good enough.
I am kind of surprised that Peter thought that would be good enough.
It is necessary, for something like this, to investigate all the
places where the code may be relying on a certain assumption, not just
assume that there's an error check everywhere that we rely on that
assumption and change only those places.

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

  6. Re: pgsql: Improve handling of parameter differences in physical replicatio

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-04-04T07:11:25Z

    On 2020-04-03 19:55, Robert Haas wrote:> I think this patch needs to be 
reverted. The only places where it
> changes anything are places where we were about to throw some error
> anyway. But as Andres's analysis shows, that's not nearly good enough.

OK, reverted.

  7. Re: pgsql: Improve handling of parameter differences in physical replicatio

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-04-14T07:17:39Z

    On 2020-03-30 20:10, Andres Freund wrote:
> Also, shouldn't dynahash be adjusted as well? There's e.g. the
> following HASH_ENTER path:
> 				/* report a generic message */
> 				if (hashp->isshared)
> 					ereport(ERROR,
> 							(errcode(ERRCODE_OUT_OF_MEMORY),
> 							 errmsg("out of shared memory")));

Could you explain further what you mean by this?  I don't understand how 
this is related.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services