Message-ID: <05bf76b1-3b00-a86a-d492-496fb81a4544@enterprisedb.com>
Date: Wed, 22 Dec 2021 20:00:09 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.0
Subject: Re: sequences vs. synchronous replication
Content-Language: en-US
To: Fujii Masao <masao.fujii@oss.nttdata.com>,
 Amit Kapila <amit.kapila16@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <712cad46-a9c8-1389-aef8-faf0203c9be9@enterprisedb.com>
 <CAA4eK1KN-Pqe1y5w0uXU2Ttn8DK4WfqgwAbCRbcDt4yzQ88wRA@mail.gmail.com>
 <4450cd74-1a41-ec03-83ee-fcabc183715d@enterprisedb.com>
 <6597cf88-a499-e5be-e027-62753b197500@oss.nttdata.com>
 <5c788a12-def6-daab-ac08-c006dfbaed7d@enterprisedb.com>
 <5774a616-8e87-d7a5-7856-6db0ff1a78b0@oss.nttdata.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <5774a616-8e87-d7a5-7856-6db0ff1a78b0@oss.nttdata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/05bf76b1-3b00-a86a-d492-496fb81a4544%40enterprisedb.com>
Precedence: bulk

On 12/22/21 18:50, Fujii Masao wrote:
> 
> 
> On 2021/12/22 21:11, Tomas Vondra wrote:
>> Interesting idea, but I think it has a couple of issues :-(
> 
> Thanks for the review!
> 
>> 1) We'd need to know the LSN of the last WAL record for any given 
>> sequence, and we'd need to communicate that between backends somehow. 
>> Which seems rather tricky to do without affecting performance.
> 
> How about using the page lsn for the sequence? nextval_internal() 
> already uses that to check whether it's less than or equal to checkpoint 
> redo location.
> 

Hmm, maybe.

> 
>> 2) SyncRepWaitForLSN() is used only in commit-like situations, and 
>> it's a simple wait, not a decision to write more WAL. Environments 
>> without sync replicas are affected by this too - yes, the data loss 
>> issue is not there, but the amount of WAL is still increased.
> 
> How about reusing only a part of code in SyncRepWaitForLSN()? Attached 
> is the PoC patch that implemented what I'm thinking.
> 
> 
>> IIRC sync_standby_names can change while a transaction is running, 
>> even just right before commit, at which point we can't just go back in 
>> time and generate WAL for sequences accessed earlier. But we still 
>> need to ensure the sequence is properly replicated.
> 
> Yes. In the PoC patch, SyncRepNeedsWait() still checks 
> sync_standbys_defined and uses SyncRepWaitMode. But they should not be 
> checked nor used because their values can be changed on the fly, as you 
> pointed out. Probably SyncRepNeedsWait() will need to be changed so that 
> it doesn't use them.
> 

Right. I think the data loss with sync standby is merely a symptom, not 
the root cause. We'd need to deduce the LSN for which to wait at commit.

> 
>> 3) I don't think it'd actually reduce the amount of WAL records in 
>> environments with many sessions (incrementing the same sequence). In 
>> those cases the WAL (generated by in-progress xact from another 
>> session) is likely to not be flushed, so we'd generate the extra WAL 
>> record. (And if the other backends would need flush LSN of this new 
>> WAL record, which would make it more likely they have to generate WAL 
>> too.)
> 
> With the PoC patch, only when previous transaction that executed 
> nextval() and caused WAL record is aborted, subsequent nextval() 
> generates additional WAL record. So this approach can reduce WAL volume 
> than other approach?
>  > In the PoC patch, to reduce WAL volume more, it might be better to make
> nextval_internal() update XactLastRecEnd and assign XID rather than 
> emitting new WAL record, when SyncRepNeedsWait() returns true.
> 

Yes, but I think there are other cases. For example the WAL might have 
been generated by another backend, in a transaction that might be still 
running. In which case I don't see how updating XactLastRecEnd in 
nextval_internal would fix this, right?

I did some experiments with increasing CACHE for the sequence, and that 
mostly eliminates the overhead. See the message I sent a couple minutes 
ago. IMHO that's a reasonable solution for the tiny number of people 
using nextval() in a way that'd be affected by this (i.e. without 
writing anything else in the xact).


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company