On Thu, Jul 13, 2023 at 8:29 PM Tomas Vondra
<tomas.von...@enterprisedb.com> wrote:
>
> On 6/23/23 15:18, Ashutosh Bapat wrote:
> > ...
> >
> > I reviewed 0001 and related parts of 0004 and 0008 in detail.
> >
> > I have only one major change request, about
> > typedef struct xl_seq_rec
> > {
> > RelFileLocator locator;
> > + bool created; /* creates a new relfilenode (CREATE/ALTER) */
> >
> > I am not sure what are the repercussions of adding a member to an existing
> > WAL
> > record. I didn't see any code which handles the old WAL format which doesn't
> > contain the "created" flag. IIUC, the logical decoding may come across
> > a WAL record written in the old format after upgrade and restart. Is
> > that not possible?
> >
>
> I don't understand why would adding a new field to xl_seq_rec be an
> issue, considering it's done in a new major version. Sure, if you
> generate WAL with old build, and start with a patched version, that
> would break things. But that's true for many other patches, and it's
> irrelevant for releases.
There are two issues
1. the name of the field "created" - what does created mean in a
"sequence status" WAL record? Consider following sequence of events
Begin;
Create sequence ('s');
select nextval('s') from generate_series(1, 1000);
...
commit
This is going to create 1000/32 WAL records with "created" = true. But
only the first one created the relfilenode. We might fix this little
annoyance by changing the name to "transactional".
2. Consider following scenario
v15 running logical decoding has restart_lsn before a "sequence
change" WAL record written in old format
stop the server
upgrade to v16
logical decoding will stat from restart_lsn pointing to a WAL record
written by v15. When it tries to read "sequence change" WAL record it
won't be able to get "created" flag.
Am I missing something here?
>
> > But I don't think it's necessary. We can add a
> > decoding routine for RM_SMGR_ID. The decoding routine will add
> > relfilelocator
> > in XLOG_SMGR_CREATE record to txn->sequences hash. Rest of the logic will
> > work
> > as is. Of course we will add non-sequence relfilelocators as well but that
> > should be fine. Creating a new relfilelocator shouldn't be a frequent
> > operation. If at all we are worried about that, we can add only the
> > relfilenodes associated with sequences to the hash table.
> >
>
> Hmmmm, that might work. I feel a bit uneasy about having to keep all
> relfilenodes, not just sequences ...
>From relfilenode it should be easy to get to rel and then see if it's
a sequence. Only add relfilenodes for the sequence.
--
Best Wishes,
Ashutosh Bapat
