Re: logical decoding and replication of sequences, take 2

On 3/10/23 11:03, John Naylor wrote:
> 
> On Wed, Mar 1, 2023 at 1:02 AM Tomas Vondra
> <tomas.vondra@enterprisedb.com <mailto:tomas.vondra@enterprisedb.com>>
> wrote:
>> here's a rebased patch to make cfbot happy, dropping the first part that
>> is now unnecessary thanks to 7fe1aa991b.
> 
> Hi Tomas,
> 
> I'm looking into doing some "in situ" testing, but for now I'll mention
> some minor nits I found:
> 
> 0001
> 
> + * so we simply do a lookup (the sequence is identified by relfilende). If
> 
> relfilenode? Or should it be called a relfilelocator, which is the
> parameter type? I see some other references to relfilenode in comments
> and commit message, and I'm not sure which need to be updated.
> 

Yeah, that's a leftover from the original patch, before the relfilenode
was renamed to relfilelocator.

> + /* XXX Maybe check that we're still in the same top-level xact? */
> 
> Any ideas on what should happen here?
> 

I don't recall why I added this comment, but I don't think there's
anything we need to do (so drop the comment).

> + /* XXX how could we have sequence change without data? */
> + if(!datalen || !tupledata)
> + elog(ERROR, "sequence decode missing tuple data");
> 
> Since the ERROR is new based on feedback, we can get rid of XXX I think.
> 
> More generally, I associate XXX comments to highlight problems or
> unpleasantness in the code that don't quite rise to the level of FIXME,
> but are perhaps more serious than "NB:", "Note:", or "Important:"
> 

Understood. I keep adding XXX in places where I have some open
questions, or something that may need to be improved (so kinda less
serious than a FIXME).

> + * When we're called via the SQL SRF there's already a transaction
> 
> I see this was copied from existing code, but I found it confusing --
> does this function have a stable name?
> 

What do you mean by "stable name"? It certainly is not exposed as a
user-callable SQL function, so I think this comment it misleading and
should be removed.

> + /* Only ever called from ReorderBufferApplySequence, so transational. */
> 
> Typo: transactional
> 
> 0002
> 
> I see a few SERIAL types in the tests but no GENERATED ... AS IDENTITY
> -- not sure if it matters, but seems good for completeness.
> 

That's a good point. Adding tests for GENERATED ... AS IDENTITY is a
good idea.

> Reminder for later: Patches 0002 and 0003 still refer to 0da92dc530,
> which is a reverted commit -- I assume it intends to refer to the
> content of 0001?
> 

Correct. That needs to be adjusted at commit time.


regards

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