[BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change - Mailing list pgsql-bugs

Simplified steps to reproduce:

Step 1: Create a table, set up a logical replication slot, and execute an `INSERT ... ON CONFLICT ... DO UPDATE` statement.

```

CREATE TABLE test_updates (id INT PRIMARY KEY, value TEXT);

SELECT * FROM pg_create_logical_replication_slot('testing_slot', 'pgoutput');

INSERT INTO test_updates (id, value) VALUES (1, 'first_insert')

ON CONFLICT(id) DO UPDATE SET value = excluded.value;

```

Step 2: Start logical decoding using `pg_logical_slot_peek_binary_changes` on the slot,

```

SELECT *

FROM pg_logical_slot_peek_binary_changes('testing_slot', NULL, NULL, 'proto_version', '1', 'publication_names', 'pub_1');

```

After Step 2, the Postgres server crashes with the assertion failure shown above.

1. In `ReorderBufferProcessTXN()`, when a `REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT`

2. Next, when a `REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM` is encountered,

the stored `specinsert` change is retrieved and applied as a regular insert change.

3. Since publication `pub_1` does not exist, an error is raised when applying the change

```

ReorderBufferProcessTXN()

  ReorderBufferApplyChange()

    → rb->apply_change()           [pgoutput_change()]

      → get_rel_sync_entry()

        → LoadPublications()

          → GetPublicationByName()

            → get_publication_oid()   ← throws ERROR here

```

4. In the `PG_CATCH` block, the `specinsert` change is cleaned up only for

Proposed Solution: Free the pending `specinsert` change in the `else` branch before calling `ReorderBufferCleanupTXN()`.

```

--- a/src/backend/replication/logical/reorderbuffer.c

+++ b/src/backend/replication/logical/reorderbuffer.c

@@ -2691,12 +2691,18 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,

                        ReorderBufferResetTXN(rb, txn, snapshot_now,

                                                                  command_id, prev_lsn,

                                                                  specinsert);

                }

                else

                {

+                       /* Return the spec insert change before cleaning up the transaction */

+                       if (specinsert != NULL)

+                       {

+                               ReorderBufferReturnChange(rb, specinsert, true);

+                               specinsert = NULL;

+                       }

                        ReorderBufferCleanupTXN(rb, txn);

                        MemoryContextSwitchTo(ecxt);

                        PG_RE_THROW();

                }

        }

        PG_END_TRY();

```

Along with this fix, we have also added a TAP test to verify in patch.

Additional Suggestion:

Currently in `PG_CATCH` block, `specinsert` is only freed in the `ERRCODE_TRANSACTION_ROLLBACK` branch

Would it make sense to move the freeing of `specinsert` before the if/else branch,

Regards,

Vishal Prasanna

Zoho Corporation