Horton

Re: logical decoding and replication of sequences, take 2

Tomas Vondra <tomas.vondra@enterprisedb.com>

View thread
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Masahiko Sawada <sawada.mshk@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Date: 2023-10-12T15:13:48Z
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 9/20/23 11:53, Dilip Kumar wrote:
> On Wed, Aug 16, 2023 at 7:57 PM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
> 
> I was reading through 0001, I noticed this comment in
> ReorderBufferSequenceIsTransactional() function
> 
> + * To decide if a sequence change should be handled as transactional or applied
> + * immediately, we track (sequence) relfilenodes created by each transaction.
> + * We don't know if the current sub-transaction was already assigned to the
> + * top-level transaction, so we need to check all transactions.
> 
> It says "We don't know if the current sub-transaction was already
> assigned to the top-level transaction, so we need to check all
> transactions". But IIRC as part of the steaming of in-progress
> transactions we have ensured that whenever we are logging the first
> change by any subtransaction we include the top transaction ID in it.
> 

Yeah, that's a stale comment - the actual code only searched through the
top-level ones (and thus relying on the immediate assignment). As I
wrote in the earlier response, I suspect this code originates from
before I added the GetCurrentTransactionId() calls.

That being said, I do wonder why with the immediate assignments we still
need the bit in ReorderBufferAssignChild that says:

    /*
     * We already saw this transaction, but initially added it to the
     * list of top-level txns.  Now that we know it's not top-level,
     * remove it from there.
     */
    dlist_delete(&subtxn->node);

I don't think that affects this patch, but it's a bit confusing.


regards

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