Re: logical decoding and replication of sequences, take 2

On 7/28/23 14:35, Ashutosh Bapat wrote:
> On Tue, Jul 25, 2023 at 10:02 PM Tomas Vondra
> <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>>
>> On 7/24/23 14:57, Ashutosh Bapat wrote:
>>> ...
>>>
>>>>
>>>>
>>>> 2) Currently, the sequences hash table is in reorderbuffer, i.e. global.
>>>> I was thinking maybe we should have it in the transaction (because we
>>>> need to do cleanup at the end). It seem a bit inconvenient, because then
>>>> we'd need to either search htabs in all subxacts, or transfer the
>>>> entries to the top-level xact (otoh, we already do that with snapshots),
>>>> and cleanup on abort.
>>>>
>>>> What do you think?
>>>
>>> Hash table per transaction seems saner design. Adding it to the top
>>> level transaction should be fine. The entry will contain an XID
>>> anyway. If we add it to every subtransaction we will need to search
>>> hash table in each of the subtransactions when deciding whether a
>>> sequence change is transactional or not. Top transaction is a
>>> reasonable trade off.
>>>
>>
>> It's not clear to me what design you're proposing, exactly.
>>
>> If we track it in top-level transactions, then we'd need copy the data
>> whenever a transaction is assigned as a child, and perhaps also remove
>> it when there's a subxact abort.
>
> I thought, esp. with your changes to assign xid, we will always know
> the top level transaction when a sequence is assigned a relfilenode.
> So the refilenodes will always get added to the correct hash directly.
> I didn't imagine a case where we will need to copy the hash table from
> sub-transaction to top transaction. If that's true, yes it's
> inconvenient.
>

Well, it's a matter of efficiency.

To check if a sequence change is transactional, we need to check if it's
for a relfilenode created in the current transaction (it can't be for
relfilenode created in a concurrent top-level transaction, due to MVCC).

If you don't copy the entries into the top-level xact, you have to walk
all subxacts and search all of those, for each sequence change. And
there may be quite a few of both subxacts and sequence changes ...

I wonder if we need to search the other top-level xacts, but we probably
need to do that. Because it might be a subxact without an assignment, or
something like that.

> As to the abort, don't we already remove entries on subtxn abort?
> Having per transaction hash table doesn't seem to change anything
> much.
>

What entries are we removing? My point is that if we copy the entries to
the top-level xact, we probably need to remove them on abort. Or we
could leave them in the top-level xact hash.

>>
>> And we'd need to still search the hashes in all toplevel transactions on
>> every sequence increment - in principle we can't have increment for a
>> sequence created in another in-progress transaction, but maybe it's just
>> not assigned yet.
>
> We hold a strong lock on sequence when changing its relfilenode. The
> sequence whose relfilenode is being changed can not be accessed by any
> concurrent transaction. So I am not able to understand what you are
> trying to say.
>

How do you know the subxact has already been recognized as such? It may
be treated as top-level transaction for a while, until the assignment.

> I think per (top level) transaction hash table is cleaner design. It
> puts the hash table where it should be. But if that makes code
> difficult, current design works too.
>

regards

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