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)

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

pá 2. 10. 2026 v 9:33 odesílatel Hayato Kuroda (Fujitsu) <
kuroda.hayato@fujitsu.com> napsal:

Hi,

This is the reply for [1]. My mailer could not receive the original post
due to
the company's policy, so I will put as the normal post. Sorry for
inconvenience.

I confirmed this could happen on PG18, PG17 and PG14. Not tested, but
expecting
for PG15 and 166 as well. This could not happen on PG19/HEAD because the
elog(ERROR) was removed by 87f7b824f20, but possible for all branches.

I think your analysis is correct. bsysscan tries to indicate that whether
we're
scanning a system table, which was turned on at systable_beginscan* and
turned
off at systable_endscan*. But if the systable scan is nested (i.e.,
pg_constraint.conbin),
the flag can be wrong reset. In PG18- the state is checked for every
getnextslot,
which raised the ERROR. In PG19+ the check is unified at the beginning
thus the
ERROR does not happen, but I guess the flag can be still wrong.

One idea is to track the depth of scans. Attached patch is for PG18, and I
tried not
to modify the header as much as possible. It also had a test code based on
your
reproducer. Can you see it's same as your expectation?

[1]:
/messages/by-id/CAF7a2M-OF+TjYUBvkSV2oyZNYuPm=EqTApcMDOC+aevBFPuWrg@mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

--
S pozdravem
Jiří Kavalík
jiri.kavalik@comgate.cz
Comgate a.s.
Gočárova třída 1754/48b, 500 02 Hradec Králové
www.comgate.cz

Hi Jiří,

Thanks for the test. I understood that no issues were found for now.

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.

Hmm, I'm not excited to modify the exposed data structure yet, unless there is a
real issue. Per my analysis, SysScanDescData only contains pointers (8 bytes),
it does not have any paddings. This meant we need to modify a size of the data
structure, it might cause failures somewhere.
Also, the existing code has the same possibility while turning on/off bsysscan,
right? So I feel it's already accepted.
(Of course, we must fix if it causes a real failure)

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi Kuroda-san,

Hmm, I'm not excited to modify the exposed data structure yet, unless

there is a

real issue.

Agreed. It was only a readability suggestion, not a fix for a real failure.
I checked again. CheckXidAlive is set only at the start of each change in
ReorderBufferProcessTXN(), before that change's catalog access. It is
cleared
only after the change loop, or on abort, where ResetLogicalStreamingState()
also resets the new depth counter. So it does not change while a scan is
open,
except on the error path, which v1 already handles. v1 looks complete to me.

Thanks for the patch.

Regards,
Jiří Kavalík

út 6. 10. 2026 v 11:22 odesílatel Hayato Kuroda (Fujitsu) <
kuroda.hayato@fujitsu.com> napsal:

Hi Jiří,

Thanks for the test. I understood that no issues were found for now.

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.

Hmm, I'm not excited to modify the exposed data structure yet, unless
there is a
real issue. Per my analysis, SysScanDescData only contains pointers (8
bytes),
it does not have any paddings. This meant we need to modify a size of the
data
structure, it might cause failures somewhere.
Also, the existing code has the same possibility while turning on/off
bsysscan,
right? So I feel it's already accepted.
(Of course, we must fix if it causes a real failure)

Best regards,
Hayato Kuroda
FUJITSU LIMITED

--
S pozdravem
Jiří Kavalík
jiri.kavalik@comgate.cz
Comgate a.s.
Gočárova třída 1754/48b, 500 02 Hradec Králové
www.comgate.cz

Hi,

On Fri, Oct 2, 2026 at 3:33 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Hi,

This is the reply for [1]. My mailer could not receive the original post due to
the company's policy, so I will put as the normal post. Sorry for inconvenience.

I confirmed this could happen on PG18, PG17 and PG14. Not tested, but expecting
for PG15 and 166 as well. This could not happen on PG19/HEAD because the
elog(ERROR) was removed by 87f7b824f20, but possible for all branches.

I think your analysis is correct. bsysscan tries to indicate that whether we're
scanning a system table, which was turned on at systable_beginscan* and turned
off at systable_endscan*. But if the systable scan is nested (i.e., pg_constraint.conbin),
the flag can be wrong reset. In PG18- the state is checked for every getnextslot,
which raised the ERROR. In PG19+ the check is unified at the beginning thus the
ERROR does not happen, but I guess the flag can be still wrong.

One idea is to track the depth of scans. Attached patch is for PG18, and I tried not
to modify the header as much as possible. It also had a test code based on your
reproducer. Can you see it's same as your expectation?

I think using a counter to fix this is OK. But I'm wondering whether it's
necessary to keep both the boolean and the counter, could we merge them into a
single parameter to simplify things?

Best Regards,
Zhijie Hou

Dear Hou,

Thanks for the review!

I think using a counter to fix this is OK. But I'm wondering whether it's
necessary to keep both the boolean and the counter, could we merge them into
a
single parameter to simplify things?

Actually it's intentional because the initially attached patch was for PG18.
bsysscan was exported and functions in tableam.h referred it. I did not want to
modify the header as much as possible thus they were retained.

For HEAD, yes, the boolean can be removed from everywhere, see attached.
I will create patched for back branches when they are stabilized.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi Kuroda-san,

For HEAD, yes, the boolean can be removed from everywhere, see attached.

One small thing in v2-HEAD: the definition in xact.c is left behind.
With the patch applied on master (061065e), "git grep -w bsysscan" still
finds

src/backend/access/transam/xact.c:103:bool bsysscan = false;

and since the extern is gone from xact.h, gcc 14 warns:

xact.c:103:17: warning: no previous declaration for 'bsysscan'
[-Wmissing-variable-declarations]

Removing that line should be enough. The comment above it in xact.c may also
want a word about the counter.

Regards,
Jiří Kavalík

čt 8. 10. 2026 v 4:36 odesílatel Hayato Kuroda (Fujitsu) <
kuroda.hayato@fujitsu.com> napsal:

Dear Hou,

Thanks for the review!

I think using a counter to fix this is OK. But I'm wondering whether it's
necessary to keep both the boolean and the counter, could we merge them

into

a
single parameter to simplify things?

Actually it's intentional because the initially attached patch was for
PG18.
bsysscan was exported and functions in tableam.h referred it. I did not
want to
modify the header as much as possible thus they were retained.

For HEAD, yes, the boolean can be removed from everywhere, see attached.
I will create patched for back branches when they are stabilized.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

--
S pozdravem
Jiří Kavalík
jiri.kavalik@comgate.cz
Comgate a.s.
Gočárova třída 1754/48b, 500 02 Hradec Králové
www.comgate.cz

Dear Jiří

Good catch. I accidentally removed the meson option " --werror" so that
I could not recognize. PSA fixed version.

Best regards,
Hayato Kuroda
FUJITSU LIMITED