From: | John Naylor <john(dot)naylor(at)enterprisedb(dot)com> |
---|---|
To: | Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> |
Cc: | vignesh C <vignesh21(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi> |
Subject: | Re: logical decoding and replication of sequences, take 2 |
Date: | 2023-03-10 10:03:04 |
Message-ID: | CAFBsxsEpknUUyYLGzpZmHOXLAL-6T5wiu-=ve_w5OgiOZ6VR7w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Mar 1, 2023 at 1:02 AM Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2023-03-10 10:21:54 | Re: Combine pg_walinspect till_end_of_wal functions with others |
Previous Message | Peter Eisentraut | 2023-03-10 09:59:12 | Re: pgsql: Use ICU by default at initdb time. |