Message-ID: <a58f1c38-10eb-7d7f-372c-cfb95007d54e@enterprisedb.com>
Date: Thu, 23 Dec 2021 19:50:22 +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>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <712cad46-a9c8-1389-aef8-faf0203c9be9@enterprisedb.com>
 <1452362.1639803168@sss.pgh.pa.us>
 <38a6a779-ee8d-de3c-fbd7-709285be88fa@enterprisedb.com>
 <7869761b-fdc7-6f7d-dc95-9ba8315a8ecf@enterprisedb.com>
 <1637476.1639862832@sss.pgh.pa.us>
 <8e24a470-b092-3c9f-0614-acd9df3f724f@enterprisedb.com>
 <f4dd81e9-fc9e-3b71-0a8a-4a5ae345331d@enterprisedb.com>
 <a5303964-717e-9cb0-52cf-7a4b7d16adc8@enterprisedb.com>
 <8231183d-1077-5394-1ffa-0c9d8c3905b0@enterprisedb.com>
 <2683973.1640048519@sss.pgh.pa.us>
 <d5d61510-eaec-469f-b9b2-0c37e28d3577@enterprisedb.com>
 <683c128a-5961-9b7b-ad68-0ea850e62363@enterprisedb.com>
 <685b21d3-254d-48a7-8a26-e6e182b5e193@oss.nttdata.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <685b21d3-254d-48a7-8a26-e6e182b5e193@oss.nttdata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/a58f1c38-10eb-7d7f-372c-cfb95007d54e%40enterprisedb.com>
Precedence: bulk

On 12/23/21 15:42, Fujii Masao wrote:
> 
> 
> On 2021/12/23 3:49, Tomas Vondra wrote:
>> Attached is a patch tweaking WAL logging - in wal_level=minimal we do 
>> the same thing as now, in higher levels we log every sequence fetch.
> 
> Thanks for the patch!
> 
> With the patch, I found that the regression test for sequences failed.
> 
> +            fetch = log = fetch;
> 
> This should be "log = fetch"?
> 
> On second thought, originally a sequence doesn't guarantee that the 
> value already returned by nextval() will never be returned by subsequent 
> nextval() after the server crash recovery. That is, nextval() may return 
> the same value across crash recovery. Is this understanding right? For 
> example, this case can happen if the server crashes after nextval() 
> returned the value but before WAL for the sequence was flushed to the 
> permanent storage.

I think the important step is commit. We don't guarantee anything for 
changes in uncommitted transactions. If you do nextval in a transaction 
and the server crashes before the WAL gets flushed before COMMIT, then 
yes, nextval may generate the same nextval again. But after commit that 
is not OK - it must not happen.

> So it's not a bug that sync standby may return the same value as
> already returned in the primary because the corresponding WAL has not
> been replicated yet, isn't it?
> 

No, I don't think so. Once the COMMIT happens (and gets confirmed by the 
sync standby), it should be possible to failover to the sync replica 
without losing any data in committed transaction. Generating duplicate 
values is a clear violation of that.

IMHO the fact that we allow a transaction to commit (even just locally) 
without flushing all the WAL it depends on is clearly a data loss bug.

> BTW, if the returned value is stored in database, the same value is 
> guaranteed not to be returned again after the server crash or by sync 
> standby. Because in that case the WAL of the transaction storing that 
> value is flushed and replicated.
> 

True, assuming the table is WAL-logged etc. I agree the issue may be 
affecting a fairly small fraction of workloads, because most people use 
sequences to generate data for inserts etc.

>> So I think this makes it acceptable / manageable. Of course, this 
>> means the values are much less monotonous (across backends), but I 
>> don't think we really promised that. And I doubt anyone is really 
>> using sequences like this (just nextval) in performance critical use 
>> cases.
> 
> I think that this approach is not acceptable to some users. So, if we 
> actually adopt WAL-logging every sequence fetch, also how about exposing 
> SEQ_LOG_VALS as reloption for a sequence? If so, those who want to log 
> every sequence fetch can set this SEQ_LOG_VALS reloption to 0. OTOH, 
> those who prefer the current behavior in spite of the risk we're 
> discussing at this thread can set the reloption to 32 like it is for 
> now, for example.
> 

I think it'd be worth explaining why you think it's not acceptable?

I've demonstrated the impact on regular workloads (with other changes 
that write stuff to WAL) is not measurable, and enabling sequence 
caching eliminates most of the overhead for the rare corner case 
workloads if needed. It does generate a bit more WAL, but the sequence 
WAL records are pretty tiny.

I'm opposed to adding relooptions that affect correctness - it just 
seems like a bad idea to me. Moreover setting the CACHE for a sequence 
does almost the same thing - if you set CACHE 32, we only generate WAL 
once every 32 increments. The only difference is that this cache is not 
shared between backends, so one backend will generate 1,2,3,... and 
another backend will generate 33,34,35,... etc. I don't think that's a 
problem, because if you want strictly monotonous / gap-less sequences 
you can't use our sequences anyway. Yes, with short-lived backends this 
may consume the sequences faster, but well - short-lived backends are 
expensive anyway and overflowing bigserial is still unlikely.

regards

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