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

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