Horton

Re: logical decoding and replication of sequences, take 2

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>, Amit Kapila <amit.kapila16@gmail.com>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Masahiko Sawada <sawada.mshk@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Date: 2024-02-21T09:22:00Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Migrate logical slots to the new node during an upgrade.

  2. Make test_decoding ddl.out shorter

  3. Fix snapshot handling in logicalmsg_decode

  4. doc: Adjust a few more references to "postmaster"

  5. Revert "Logical decoding of sequences"

On Wed, Feb 21, 2024 at 2:43 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> So the problem is that we might consider the transaction change as
> non-transaction and mark this flag as true.

But it's not "might" right? It's absolutely 100% certain that we will
consider that transaction's changes as non-transactional ... because
when we're in fast-forward mode, the table of new relfilenodes is not
built, and so whenever we check whether any transaction made a new
relfilenode for this sequence, the answer will be no.

> But what would have
> happened if we would have identified it correctly as transactional?
> In such cases, we wouldn't have set this flag here but then we would
> have set this while processing the DecodeAbort/DecodeCommit, so the
> net effect would be the same no?  You may question what if the
> Abort/Commit WAL never appears in the WAL, but this flag is
> specifically for the upgrade case, and in that case we have to do a
> clean shutdown so may not be an issue.  But in the future, if we try
> to use 'ctx->processing_required' for something else where the clean
> shutdown is not guaranteed then this flag can be set incorrectly.
>
> I am not arguing that this is a perfect design but I am just making a
> point about why it would work.

Even if this argument is correct (and I don't know if it is), the code
and comments need some updating. We should not be testing a flag that
is guaranteed false with comments that make it sound like the value of
the flag is trustworthy when it isn't.

-- 
Robert Haas
EDB: http://www.enterprisedb.com