On Wed, Mar 1, 2023 at 1:02 AM Tomas Vondra <tomas.von...@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. + /* XXX Maybe check that we're still in the same top-level xact? */ Any ideas on what should happen here? + /* 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:" + * 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? + /* 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. 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? -- John Naylor EDB: http://www.enterprisedb.com