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
