Re: Streaming decoding fails with "unexpected table_index_fetch_tuple call during logical decoding" when a relation has a TOASTed conbin (follow-up to BUG #18641) - Mailing list pgsql-bugs

Hi Kuroda-san,

Thank you for the quick analysis and the patch.

> Can you see it's same as your expectation?

Yes. I tested v1 on REL_18_STABLE (37bbf5bba0, 18.6 plus later commits), with
two builds from the same tree differing only by the patch, both configured
with --enable-debug --enable-cassert.

In each case below, a fresh backend peeks at a slot while a transaction that
first inserts one row into the test table, then 10000 rows into a filler
table, is still open (logical_decoding_work_mem = 64kB, CHECKPOINT before the
peek):

1. TOASTed conbin first, three more constraints after it:
   - test_decoding: unpatched ERROR, patched OK
   - pgoutput, table published: unpatched ERROR, patched OK
   - pgoutput, table NOT published: unpatched ERROR, patched OK

2. TOASTed conbin first, only the NOT NULL and PK constraints after it:
   - test_decoding: unpatched ERROR, patched OK
   - pgoutput, table published: unpatched ERROR, patched OK
   - pgoutput, table NOT published: unpatched ERROR, patched OK

3. TOASTed conbin last: OK on both builds, both plugins.

4. No TOASTed conbin: OK on both builds, both plugins.

"OK" means the whole transaction was decoded (10098 rows from test_decoding,
10100 from pgoutput). There were no assertion failures in either server log.

Your new stream.sql test fails on the unpatched build with the same ERROR and
passes with the patch. With the patch, contrib/test_decoding "make check"
(19 regression + 13 isolation tests) and the core "make check" (231 tests)
pass.

The "table NOT published" rows may be worth noting: the relation that hit the
error in our production case is not in the subscribed publication, it is only
modified by the streamed transaction. The relcache entry is still built, so
the bug is reachable for any table the transaction touches.

One question while reading the patch, not a problem I could trigger: the depth
is incremented in systable_beginscan* and decremented in systable_endscan* only
if CheckXidAlive is valid, and that is evaluated separately at each end. If
CheckXidAlive changed while a scan was open, the counter would be off by one.
I could not find a path where that happens. Error paths look fine, since
AbortTransaction/AbortSubTransaction call ResetLogicalStreamingState(). If a
SysScanDesc field is acceptable despite the header concern, remembering in the
scan whether it was counted would make the pairing explicit.

Regards,
Jiří Kavalík