Content-Type: multipart/mixed; boundary="------------za50u3jm0E2UwPeGC6SZEaYT"
Message-ID: <84d60912-6eab-9b84-5de3-41765a5449e8@enterprisedb.com>
Date: Thu, 17 Nov 2022 22:13:23 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
Subject: Re: logical decoding and replication of sequences, take 2
Content-Language: en-US
To: Andres Freund <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <76e5fcd8-8072-8ea2-d361-2e811941000c@enterprisedb.com>
 <c5e671bc-3400-c2f4-14d3-1ae06c462dad@enterprisedb.com>
 <8bf1c518-b886-fe1b-5c42-09f9c663146d@enterprisedb.com>
 <CA+TgmoaYG7672OgdwpGm5cOwy8_ftbs=3u-YMvR9fiJwQUzgrQ@mail.gmail.com>
 <110816d7-ebe1-3612-8ca7-d38489dd8e22@enterprisedb.com>
 <20221117024357.ljjme6v75mny2j6u@awork3.anarazel.de>
 <29c4a08e-3610-f338-3e35-75882dcf759e@enterprisedb.com>
 <20221117170716.5da7set7fy3t6mfx@awork3.anarazel.de>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <20221117170716.5da7set7fy3t6mfx@awork3.anarazel.de>
Archived-At: 
 <https://www.postgresql.org/message-id/84d60912-6eab-9b84-5de3-41765a5449e8%40enterprisedb.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------za50u3jm0E2UwPeGC6SZEaYT
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 11/17/22 18:07, Andres Freund wrote:
> Hi,
> 
> On 2022-11-17 12:39:49 +0100, Tomas Vondra wrote:
>> On 11/17/22 03:43, Andres Freund wrote:
>>> On 2022-11-17 02:41:14 +0100, Tomas Vondra wrote:
>>>> Well, yeah - we can either try to perform the stuff independently of the
>>>> transactions that triggered it, or we can try making it part of some of
>>>> the transactions. Each of those options has problems, though :-(
>>>>
>>>> The first version of the patch tried the first approach, i.e. decode the
>>>> increments and apply that independently. But:
>>>>
>>>>   (a) What would you do with increments of sequences created/reset in a
>>>>       transaction? Can't apply those outside the transaction, because it
>>>>       might be rolled back (and that state is not visible on primary).
>>>
>>> I think a reasonable approach could be to actually perform different WAL
>>> logging for that case. It'll require a bit of machinery, but could actually
>>> result in *less* WAL logging overall, because we don't need to emit a WAL
>>> record for each SEQ_LOG_VALS sequence values.
>>>
>>
>> Could you elaborate? Hard to comment without knowing more ...
>>
>> My point was that stuff like this (creating a new sequence or at least a
>> new relfilenode) means we can't apply that independently of the
>> transaction (unlike regular increments). I'm not sure how a change to
>> WAL logging would make that go away.
> 
> Different WAL logging would make it easy to handle that on the logical
> decoding level. We don't need to emit WAL records each time a
> created-in-this-toplevel-xact sequences gets incremented as they're not
> persisting anyway if the surrounding xact aborts. We already need to remember
> the filenode so it can be dropped at the end of the transaction, so we could
> emit a single record for each sequence at that point.
> 
> 
>>>>   (b) What about increments created before we have a proper snapshot?
>>>>       There may be transactions dependent on the increment. This is what
>>>>       ultimately led to revert of the patch.
>>>
>>> I don't understand this - why would we ever need to process those increments
>>> from before we have a snapshot?  Wouldn't they, by definition, be before the
>>> slot was active?
>>>
>>> To me this is the rough equivalent of logical decoding not giving the initial
>>> state of all tables. You need some process outside of logical decoding to get
>>> that (obviously we have some support for that via the exported data snapshot
>>> during slot creation).
>>>
>>
>> Which is what already happens during tablesync, no? We more or less copy
>> sequences as if they were tables.
> 
> I think you might have to copy sequences after tables, but I'm not sure. But
> otherwise, yea.
> 
> 
>>> I assume that part of the initial sync would have to be a new sequence
>>> synchronization step that reads all the sequence states on the publisher and
>>> ensures that the subscriber sequences are at the same point. There's a bit of
>>> trickiness there, but it seems entirely doable. The logical replication replay
>>> support for sequences will have to be a bit careful about not decreasing the
>>> subscriber's sequence values - the standby initially will be ahead of the
>>> increments we'll see in the WAL. But that seems inevitable given the
>>> non-transactional nature of sequences.
>>>
>>
>> See fetch_sequence_data / copy_sequence in the patch. The bit about
>> ensuring the sequence does not go away (say, using page LSN and/or LSN
>> of the increment) is not there, however isn't that pretty much what I
>> proposed doing for "reconciling" the sequence state logged at COMMIT?
> 
> Well, I think the approach of logging all sequence increments at commit is the
> wrong idea...
> 

But we're not logging all sequence increments, no?

We're logging the state for each sequence touched by the transaction,
but only once - if the transaction incremented the sequence 1000000x
times, we'll still log it just once (at least for this particular purpose).

Yes, if transactions touch each sequence just once, then we're logging
individual increments.

The only more efficient solution would be to decode the existing WAL
(every ~32 increments), and perhaps also tracking which sequences were
accessed by a transaction. And then simply stashing the increments in a
global reorderbuffer hash table, and then applying only the last one at
commit time. This would require the transactional / non-transactional
behavior (I think), but perhaps we can make that work.

Or are you thinking about some other scheme?

> Creating a new relfilenode whenever a sequence is incremented seems like a
> complete no-go to me. That increases sequence overhead by several orders of
> magnitude and will lead to *awful* catalog bloat on the subscriber.
> 

You mean on the the apply side? Yes, I agree this needs a better
approach, I've focused on the decoding side so far.

> 
>>>
>>>> This version of the patch tries to do the opposite thing - make sure
>>>> that the state after each commit matches what the transaction might have
>>>> seen (for sequences it accessed). It's imperfect, because it might log a
>>>> state generated "after" the sequence got accessed - it focuses on the
>>>> guarantee not to generate duplicate values.
>>>
>>> That approach seems quite wrong to me.
>>>
>>
>> Why? Because it might log a state for sequence as of COMMIT, when the
>> transaction accessed the sequence much earlier?
> 
> Mainly because sequences aren't transactional and trying to make them will
> require awful contortions.
> 
> While there are cases where we don't flush the WAL / wait for syncrep for
> sequences, we do replicate their state correctly on physical replication. If
> an LSN has been acknowledged as having been replicated, we won't just loose a
> prior sequence increment after promotion, even if the transaction didn't [yet]
> commit.
> 

True, I agree we should aim to achieve that.

> It's completely valid for an application to call nextval() in one transaction,
> potentially even abort it, and then only use that sequence value in another
> transaction.
> 

I don't quite agree with that - we make no promises about what happens
to sequence changes in aborted transactions. I don't think I've ever
seen an application using such pattern either.

And I'd argue we already fail to uphold such guarantee, because we don't
wait for syncrep if the sequence WAL happened in aborted transaction. So
if you use the value elsewhere (outside PG), you may lose it.

Anyway, I think the scheme I outlined above (with stashing decoded
increments, logged once every 32 values) and applying the latest
increment for all sequences at commit, would work.

> 
> 
>>> I did some skimming of the referenced thread about the reversal of the last
>>> approach, but I couldn't really understand what the fundamental issues were
>>> with the reverted implementation - it's a very long thread and references
>>> other threads.
>>>
>>
>> Yes, it's long/complex, but I intentionally linked to a specific message
>> which describes the issue ...
>>
>> It's entirely possible there is a simple fix for the issue, and I just
>> got confused / unable to see the solution. The whole issue was due to
>> having a mix of transactional and non-transactional cases, similarly to
>> logical messages - and logicalmsg_decode() has the same issue, so maybe
>> let's talk about that for a moment.
>>
>> See [3] and imagine you're dealing with a transactional message, but
>> you're still building a consistent snapshot. So the first branch applies:
>>
>>     if (transactional &&
>>         !SnapBuildProcessChange(builder, xid, buf->origptr))
>>         return;
>>
>> but because we don't have a snapshot, SnapBuildProcessChange does this:
>>
>>     if (builder->state < SNAPBUILD_FULL_SNAPSHOT)
>>         return false;
> 
> In this case we'd just return without further work in logicalmsg_decode(). The
> problematic case presumably is is when we have a full snapshot but aren't yet
> consistent, but xid is >= next_phase_at. Then SnapBuildProcessChange() returns
> true. And we reach:
> 
>> which however means logicalmsg_decode() does
>>
>>     snapshot = SnapBuildGetOrBuildSnapshot(builder);
>>
>> which crashes, because it hits this assert:
>>
>>     Assert(builder->state == SNAPBUILD_CONSISTENT);
> 
> I think the problem here is just that we shouldn't even try to get a snapshot
> in the transactional case - note that it's not even used in
> ReorderBufferQueueMessage() for transactional message. The transactional case
> needs to behave like a "normal" change - we might never decode the message if
> the transaction ends up committing before we've reached a consistent point.
> 
> 
>> The sequence decoding did almost the same thing, with the same issue.
>> Maybe the correct thing to do is to just ignore the change in this case?
> 
> No, I don't think that'd be correct, the message | sequence needs to be queued
> for the transaction. If the transaction ends up committing after we've reached
> consistency, we'll get the correct snapshot from the base snapshot set in
> SnapBuildProcessChange().
> 

Yeah, I think you're right. I looked at this again, with fresh mind, and
I came to the same conclusion. Roughly what the attached patch does.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--------------za50u3jm0E2UwPeGC6SZEaYT
Content-Type: text/x-patch; charset=UTF-8; name="logical-msg-fix.patch"
Content-Disposition: attachment; filename="logical-msg-fix.patch"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvZGVjb2RlLmMg
Yi9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9sb2dpY2FsL2RlY29kZS5jCmluZGV4IDJjYzBh
YzllYjA5Li42Njc3MWFjMzVhMiAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvcmVwbGljYXRp
b24vbG9naWNhbC9kZWNvZGUuYworKysgYi9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9sb2dp
Y2FsL2RlY29kZS5jCkBAIC01NjQsNyArNTY0LDYgQEAgbG9naWNhbG1zZ19kZWNvZGUoTG9n
aWNhbERlY29kaW5nQ29udGV4dCAqY3R4LCBYTG9nUmVjb3JkQnVmZmVyICpidWYpCiAJVHJh
bnNhY3Rpb25JZCB4aWQgPSBYTG9nUmVjR2V0WGlkKHIpOwogCXVpbnQ4CQlpbmZvID0gWExv
Z1JlY0dldEluZm8ocikgJiB+WExSX0lORk9fTUFTSzsKIAlSZXBPcmlnaW5JZCBvcmlnaW5f
aWQgPSBYTG9nUmVjR2V0T3JpZ2luKHIpOwotCVNuYXBzaG90CXNuYXBzaG90OwogCXhsX2xv
Z2ljYWxfbWVzc2FnZSAqbWVzc2FnZTsKIAogCWlmIChpbmZvICE9IFhMT0dfTE9HSUNBTF9N
RVNTQUdFKQpAQCAtNTk0LDggKzU5Myw3IEBAIGxvZ2ljYWxtc2dfZGVjb2RlKExvZ2ljYWxE
ZWNvZGluZ0NvbnRleHQgKmN0eCwgWExvZ1JlY29yZEJ1ZmZlciAqYnVmKQogCQkJICBTbmFw
QnVpbGRYYWN0TmVlZHNTa2lwKGJ1aWxkZXIsIGJ1Zi0+b3JpZ3B0cikpKQogCQlyZXR1cm47
CiAKLQlzbmFwc2hvdCA9IFNuYXBCdWlsZEdldE9yQnVpbGRTbmFwc2hvdChidWlsZGVyKTsK
LQlSZW9yZGVyQnVmZmVyUXVldWVNZXNzYWdlKGN0eC0+cmVvcmRlciwgeGlkLCBzbmFwc2hv
dCwgYnVmLT5lbmRwdHIsCisJUmVvcmRlckJ1ZmZlclF1ZXVlTWVzc2FnZShjdHgtPnJlb3Jk
ZXIsIHhpZCwgYnVmLT5lbmRwdHIsCiAJCQkJCQkJICBtZXNzYWdlLT50cmFuc2FjdGlvbmFs
LAogCQkJCQkJCSAgbWVzc2FnZS0+bWVzc2FnZSwgLyogZmlyc3QgcGFydCBvZiBtZXNzYWdl
IGlzCiAJCQkJCQkJCQkJCQkgKiBwcmVmaXggKi8KZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5k
L3JlcGxpY2F0aW9uL2xvZ2ljYWwvcmVvcmRlcmJ1ZmZlci5jIGIvc3JjL2JhY2tlbmQvcmVw
bGljYXRpb24vbG9naWNhbC9yZW9yZGVyYnVmZmVyLmMKaW5kZXggMzFmNzM4MWYyZDYuLjBh
MzRhZWJkZDU4IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9sb2dpY2Fs
L3Jlb3JkZXJidWZmZXIuYworKysgYi9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9sb2dpY2Fs
L3Jlb3JkZXJidWZmZXIuYwpAQCAtODE4LDcgKzgxOCw3IEBAIFJlb3JkZXJCdWZmZXJRdWV1
ZUNoYW5nZShSZW9yZGVyQnVmZmVyICpyYiwgVHJhbnNhY3Rpb25JZCB4aWQsIFhMb2dSZWNQ
dHIgbHNuLAogICovCiB2b2lkCiBSZW9yZGVyQnVmZmVyUXVldWVNZXNzYWdlKFJlb3JkZXJC
dWZmZXIgKnJiLCBUcmFuc2FjdGlvbklkIHhpZCwKLQkJCQkJCSAgU25hcHNob3Qgc25hcCwg
WExvZ1JlY1B0ciBsc24sCisJCQkJCQkgIFhMb2dSZWNQdHIgbHNuLAogCQkJCQkJICBib29s
IHRyYW5zYWN0aW9uYWwsIGNvbnN0IGNoYXIgKnByZWZpeCwKIAkJCQkJCSAgU2l6ZSBtZXNz
YWdlX3NpemUsIGNvbnN0IGNoYXIgKm1lc3NhZ2UpCiB7CkBAIC04NDUsNyArODQ1LDExIEBA
IFJlb3JkZXJCdWZmZXJRdWV1ZU1lc3NhZ2UoUmVvcmRlckJ1ZmZlciAqcmIsIFRyYW5zYWN0
aW9uSWQgeGlkLAogCWVsc2UKIAl7CiAJCVJlb3JkZXJCdWZmZXJUWE4gKnR4biA9IE5VTEw7
Ci0JCXZvbGF0aWxlIFNuYXBzaG90IHNuYXBzaG90X25vdyA9IHNuYXA7CisKKwkJTG9naWNh
bERlY29kaW5nQ29udGV4dCAqY3R4ID0gcmItPnByaXZhdGVfZGF0YTsKKwkJU25hcEJ1aWxk
ICAqYnVpbGRlciA9IGN0eC0+c25hcHNob3RfYnVpbGRlcjsKKworCQl2b2xhdGlsZSBTbmFw
c2hvdCBzbmFwc2hvdF9ub3cgPSBTbmFwQnVpbGRHZXRPckJ1aWxkU25hcHNob3QoYnVpbGRl
cik7CiAKIAkJaWYgKHhpZCAhPSBJbnZhbGlkVHJhbnNhY3Rpb25JZCkKIAkJCXR4biA9IFJl
b3JkZXJCdWZmZXJUWE5CeVhpZChyYiwgeGlkLCB0cnVlLCBOVUxMLCBsc24sIHRydWUpOwpk
aWZmIC0tZ2l0IGEvc3JjL2luY2x1ZGUvcmVwbGljYXRpb24vcmVvcmRlcmJ1ZmZlci5oIGIv
c3JjL2luY2x1ZGUvcmVwbGljYXRpb24vcmVvcmRlcmJ1ZmZlci5oCmluZGV4IGIyM2Q4Y2M0
ZjlmLi41MGI0OGQ1MmI1YSAxMDA2NDQKLS0tIGEvc3JjL2luY2x1ZGUvcmVwbGljYXRpb24v
cmVvcmRlcmJ1ZmZlci5oCisrKyBiL3NyYy9pbmNsdWRlL3JlcGxpY2F0aW9uL3Jlb3JkZXJi
dWZmZXIuaApAQCAtNjQ2LDcgKzY0Niw3IEBAIGV4dGVybiB2b2lkIFJlb3JkZXJCdWZmZXJR
dWV1ZUNoYW5nZShSZW9yZGVyQnVmZmVyICpyYiwgVHJhbnNhY3Rpb25JZCB4aWQsCiAJCQkJ
CQkJCQkgWExvZ1JlY1B0ciBsc24sIFJlb3JkZXJCdWZmZXJDaGFuZ2UgKmNoYW5nZSwKIAkJ
CQkJCQkJCSBib29sIHRvYXN0X2luc2VydCk7CiBleHRlcm4gdm9pZCBSZW9yZGVyQnVmZmVy
UXVldWVNZXNzYWdlKFJlb3JkZXJCdWZmZXIgKnJiLCBUcmFuc2FjdGlvbklkIHhpZCwKLQkJ
CQkJCQkJCSAgU25hcHNob3Qgc25hcCwgWExvZ1JlY1B0ciBsc24sCisJCQkJCQkJCQkgIFhM
b2dSZWNQdHIgbHNuLAogCQkJCQkJCQkJICBib29sIHRyYW5zYWN0aW9uYWwsIGNvbnN0IGNo
YXIgKnByZWZpeCwKIAkJCQkJCQkJCSAgU2l6ZSBtZXNzYWdlX3NpemUsIGNvbnN0IGNoYXIg
Km1lc3NhZ2UpOwogZXh0ZXJuIHZvaWQgUmVvcmRlckJ1ZmZlckNvbW1pdChSZW9yZGVyQnVm
ZmVyICpyYiwgVHJhbnNhY3Rpb25JZCB4aWQsCg==

--------------za50u3jm0E2UwPeGC6SZEaYT--