Re: logical decoding and replication of sequences, take 2

On Tue, Jul 25, 2023 at 10:02 PM Tomas Vondra
<tomas.vondra@enterprisedb.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.

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.

>
> 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.

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.

-- 
Best Wishes,
Ashutosh Bapat