Thread: Re:BUG #18369: logical decoding core on AssertTXNLsnOrder()

This issue exists in PG 12 -15.

At 2024-02-28 15:57:37, "ocean_li_996" <ocean_li_996@163.com> wrote:

At 2024-02-28 15:53:30, "PG Bug reporting form" <noreply@postgresql.org> wrote:

>1) The WAL records from restart_lsn to the corresponding lsn when the issue
>occurred,
>2) personal analysis of the problem,
>3) the steps to reproduce the issue,
>4) personal proposed solution
>will be posted later under this thread.
>

Please see the similar issue discussed last year:
https://www.postgresql.org/message-id/f158d9ca-2057-2836-a522-0b1278be5a53%40gmail.com

With your patch applied (on REL_14_STABLE) I still get:
TRAP: FailedAssertion("builder->next_phase_at == InvalidTransactionId", File: "snapbuild.c", Line: 1623, PID: 92772)

when running make -s installcheck-force -C contrib/test_decoding...
as specified in that message.
(You might need to disable REGRESS tests in the Makefile to reach
isolation tests.)

Best regards,
Alexander

> With your patch applied (on REL_14_STABLE) I still get:

> TRAP: FailedAssertion("builder->next_phase_at == InvalidTransactionId", File: "snapbuild.c", Line: 1623, PID: 92772)
>
> when running make -s installcheck-force -C contrib/test_decoding...
> as specified in that message.
> (You might need to disable REGRESS tests in the Makefile to reach
> isolation tests.)

I'm not sure if I fully get your mind. I disabled the REGRESS tests in the test_decoding/Makefile and then manually run the command "make -s installcheck-force -C contrib/test_decoding" a few times. I didn't encounter the issue you mentioned. Is this problem consistently reproducible in your environment? BTW, the  issue mentioned in [1] is more similar to your problem. And the patch was not applied to v14.  Maybe it is another issue.

You can try the following script (assuming a server with the test_decoding
module installed is running):
rm -rf contrib/test_decoding_*
numclients=5
for ((c=1;c<=numclients;c++)); do
   cp -r contrib/test_decoding contrib/test_decoding_$c
   sed "s/REGRESS = /# REGRESS =/" -i contrib/test_decoding_$c/Makefile
   sed "s/isolation_slot/isolation_slot_$c/" -i contrib/test_decoding_$c/specs/catalog_change_snapshot.spec # Use independent slots
   sed "$(printf '$p; %.0s' `seq 50`)" -i contrib/test_decoding_$c/specs/catalog_change_snapshot.spec # Repeat the last permutation 50 times
done
for ((c=1;c<=numclients;c++)); do
   EXTRA_REGRESS_OPTS="--dbname=regress_$c" make -s installcheck-force -C contrib/test_decoding_$c USE_MODULE_DB=1  >"installcheck-$c.log" 2>&1 &
done
wait

Though that's really not directly related to the current issue (sorry for
the wrong direction, my point was that there are still living bugs in this
area).

I've found that your added test case fails on REL_15_STABLE starting from
b793a416b (6b77048e5 on REL_14_STABLE), so it looks like this is a new
defect introduced in REL_14_STABLE, REL_15_STABLE recently with the fix for
bug #18280.

As to REL_13_STABLE/REL_12_STABLE the failure reproduced starting from
commits 38dbaaf27/02600886c, a result of the aforementioned discussion:
https://www.postgresql.org/message-id/CAA4eK1Lx%3Dg09z2k9Teq9ca1eRzfpfxJwFdjyHNwgEKv69KWhrQ%40mail.gmail.com

Best regards,
Alexander

@font-face {font-family:"Cambria Math"; panose-1:2 4 5 3 5 4 6 3 2 4;}@font-face {font-family:游ゴシック; panose-1:2 11 4 0 0 0 0 0 0 0;}@font-face {font-family:"ＭＳ Ｐゴシック"; panose-1:2 11 6 0 7 2 5 8 2 4;}@font-face {font-family:"\@ＭＳ Ｐゴシック";}@font-face {font-family:"\@游ゴシック"; panose-1:2 11 4 0 0 0 0 0 0 0;}p.MsoNormal, li.MsoNormal, div.MsoNormal {margin:0mm; font-size:12.0pt; font-family:"ＭＳ Ｐゴシック";}.MsoChpDefault {mso-style-type:export-only; font-size:10.0pt; mso-ligatures:none;}div.WordSection1 {page:WordSection1;}

Dear Alexander,

 

I also ran your script after applying patches on REL_14_STABLE, but I got another failure.

I have not analyzed yet so I share the result as-is.

Was I missing something?

```

#0  0x00007f4f033de387 in raise () from /lib64/libc.so.6

#1  0x00007f4f033dfa78 in abort () from /lib64/libc.so.6

#2  0x0000000000b08152 in ExceptionalCondition (conditionName=0xcc8228 "prev_first_lsn < cur_txn->first_lsn",

    errorType=0xcc8024 "FailedAssertion", fileName=0xcc8070 "reorderbuffer.c", lineNumber=916) at assert.c:69

#3  0x00000000008e01ad in AssertTXNLsnOrder (rb=0x19a95c0) at reorderbuffer.c:916

#4  0x00000000008dfae8 in ReorderBufferTXNByXid (rb=0x19a95c0, xid=1753, create=true, is_new=0x0, lsn=37430336,

    create_as_top=true) at reorderbuffer.c:669

Yes, you're right. That script produces another failure as I mentioned
upthread, but still in AssertTXNLsnOrder().
I had thought that the fix should cover that case too, but then I found out
that the defect in question is rather new, so maybe the more focused fix is
really preferable.

Best regards,
Alexander

Dear Haiyang Li,

Thanks for reporting. I could reproduce the  issue on PG12-15.
I thought you have already known the reason, but let me share my analysis for
the confirmation. The root cause is missing consideration for temporary tables.

## Premise

INSERT/UPDATE/DELETE operations for temporary tables are not recorded to WAL files,
but xid would be involved for such operations.
So there is a possibility that a transaction does not have related WAL records
if only temp tables are modified within the transaction.

Basically such transactions would not be decoded.

## Found issue

### Empty transaction is decoded on PG14 and PG15

However, there is a room for generating ReorderBufferTxn for empty transactions,
which was introduced by 6b77048e5. Conditions are:

1. There are sub transactions which modify only temp tables, and
2. the top transaction modifies the catalog.

The call-stack toward the generation is below.

```
ReorderBufferTXNByXid(create = true, create_as_top = true)
ReorderBufferXidSetCatalogChanges()    // for sub transactions
SnapBuildXidSetCatalogChanges()          // for top transaction
DecodeCommit()                                      // for top transaction
```

The path has been introduced by 6b77048e5.
Previously, calling ReorderBufferXidSetCatalogChanges() for sub transactions
would be skipped, if they do not have catalog changes or they have not decoded yet.
However, the commit ensures sub transactions must be marked as containing
catalog changes, and this also enforces to decode transactions even if it is
empty.

### Assertion failure

The empty transactions would be created as top transactions. At that time,
AssertTXNLsnOrder() is called so that we ensured that first_lsn of top-transactions
must be strictly higher than previous. But they can be the same if there are more
than two empty transactions. It led an assertion failure.

### Considerations on PG12 and PG13

Same failure can be occurred on the PG12 and 13, and the background is bit different.
343afa967 removed a ReorderBufferAssignChild() from SnapBuildXidSetCatalogChanges().
The function allowed empty transactions being marked as sub-trans, so there had
been no problem in past. After the commit, assignments were removed, so that the
empty transactions would be generated as top-transactions.

## Possible solutions

I think there are several solutions.
Note that I assumed here that fixes for all the versions should be almost the same.

* Ease the condition in AssertTXNLsnOrder(). If the decoded transaction is empty,
  it can be allowed that the first_lsn is same as previous one.
  PSA file to see my consideration.
* Generate a ReorderBufferTXN as sub transaction when we are in this path.
  The approach has already been shared by you. However, note that this needs to
  extend the ReorderBufferXidSetCatalogChanges function, and breaks ABI
  compatibility [1].
* Avoid calling ReorderBufferXidSetCatalogChanges() if the target transaction
  has not been decoded. An concern is that ReorderBuffer does not provide an API
  for checking whether the transaction has been already decoded or not.

I will keep analyzing more and share further updates if found.
Thought?

[1]: https://wiki.postgresql.org/wiki/Committing_checklist#Maintaining_ABI_compatibility_while_backpatching

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/

Dear Haiyang, Alexander,

>LGFM.  For my observation, the most case failed on AsserTXNOrder is checking empty
>decoded transaction. Maybe we should pay more attention to review ReorderBufferTXNIsEmpty.

While checking on my fresh eyes, I thought the code might be wrong. The first_lsn
would be same as previous one, when the *PREVIOUS transaction* (not cur_txn) was
empty. Thought?

Also I found the crash reported on [1] was not resolved by the patch. I'm
still analyzing but I have not found the good reproducer yet which could be done
by spec file. Can someone find the workload?

[1]:
https://www.postgresql.org/message-id/TYCPR01MB12077573479C5A2471BDE8065F5232%40TYCPR01MB12077.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/

Dear Haiyang, Alexander,

I analyzed the second failure reported in [1]. The failure happened on the all
Supported branches. Attached patches fixes two failures [1] [2] on PG12-PG15,
and only the failure [1] on PG16-HEAD.

Below part describes the second failure. The issue would be occurred when:

1) Logical decoding starts from the middle of a sub-transaction, and
2) NEW_CID record is initially decoded in the sub-transaction, and
3) An arbitrary changes are decoded in the sub-transaction.

## Stuck trace

Just in case, below is a stuck trace I got.

```
#0  0x00007f85e64af387 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:55
#1  0x00007f85e64b0a78 in __GI_abort () at abort.c:90
#2  0x0000000000b082d3 in ExceptionalCondition (conditionName=0xcc83e7 "prev_txn_is_empty",
    errorType=0xcc8184 "FailedAssertion", fileName=0xcc81d0 "reorderbuffer.c", lineNumber=932) at assert.c:69
#3  0x00000000008e023d in AssertTXNLsnOrder (rb=0x1a69e00) at reorderbuffer.c:932
#4  0x00000000008e3cf5 in ReorderBufferSetBaseSnapshot (rb=0x1a69e00, xid=757, lsn=24749472, snap=0x1a660e8)
    at reorderbuffer.c:3140
#5  0x00000000008e910a in SnapBuildProcessChange (builder=0x1a65de0, xid=757, lsn=24749472) at snapbuild.c:799
#6  0x00000000008d2740 in DecodeHeapOp (ctx=0x1a53d60, buf=0x7ffe24280460) at decode.c:521
#7  0x00000000008d1df7 in LogicalDecodingProcessRecord (ctx=0x1a53d60, record=0x1a54160) at decode.c:154
#8  0x00000000008d9766 in pg_logical_slot_get_changes_guts (fcinfo=0x1a41f20, confirm=true, binary=false)
    at logicalfuncs.c:296
#9  0x00000000008d989b in pg_logical_slot_get_changes (fcinfo=0x1a41f20) at logicalfuncs.c:365
...
```

## Reproducer

Each attached patches contained the reproducer.
As I said above, logical decoding is started from the middle of the sub-transaction
initial decoded record must be NEW_CID. Below shows the wal records from the
decoding start point.

```
$ pg_waldump tmp_check_iso/data/pg_wal/000000010000000000000001 -s 0/179A4E8
rmgr: ..., tx:          0, lsn: 0/0179A4E8, prev 0/0179A4A8, desc: CHECKPOINT_ONLINE...
rmgr: ..., tx:        757, lsn: 0/0179A560, prev 0/0179A4E8, desc: NEW_CID ...
rmgr: ..., tx:        757, lsn: 0/0179A5A0, prev 0/0179A560, desc: INSERT+INIT...
rmgr: ..., tx:        756, lsn: 0/0179A5E0, prev 0/0179A5A0, desc: COMMIT 2024-03-07 13:54:56.243746 UTC; subxacts: 757
rmgr: ..., tx:          0, lsn: 0/0179A618, prev 0/0179A5E0, desc: RUNNING_XACTS ...
```

## Analysis

SnapBuildProcessNewCid() generates a two transactions (xid = 757 and 756) based on
the same wal record, so both entries have same first_lsn in ReorderBufferTXN.
However, they are not associated as top-sub relationship so that they are pushed to
toplevel_by_lsn list. Since AssertTXNLsnOrder() does not assumes the case which
two entries have same first_lsn, it leads an assertion failure. Note that the
sub-transaction is generated earlier than top one and sub is not an empty, an
initial fix was not sufficient.

### Detailed analysis

I added a debug variable in AssertTXNLsnOrder to preserve the previous entry in
toplevel_by_lsn loop, and I found that two ReorderBufferTNXes have same first_lsn.
Also, according to above, 757 is a sub-transaction of 756.

```
(gdb) p *prev
$1 = {txn_flags = 1, xid = 757, toplevel_xid = 0, gid = 0x0, first_lsn = 24749408, ...nentries = 1, ..., ntuplecids =
0,...}
(gdb) p *cur_txn
$2 = {txn_flags = 0, xid = 756, toplevel_xid = 0, gid = 0x0, first_lsn = 24749408, ...nentries = 0, ..., ntuplecids =
1,...}
```

Based on above and some debug outputs, I considered a scenario. Below flow showed
a case when the sub-transaction (xid = 757) was decoded.

```
DecodeHeap2Op(info = XLOG_HEAP2_NEW_CID)
    SnapBuildProcessNewCid()
        ReorderBufferXidSetCatalogChanges(xid, lsn)
            ReorderBufferTXNByXid(xid, lsn)
                -> A ReorderBufferTXN for subtransaciton (757) was generated.
                   The first_lsn was the head of NEW_CID record.
        ReorderBufferAddNewCommandId(xid, lsn)
            ReorderBufferQueueChange(xid, lsn)
                -> A ReorderBufferChange was queued to the subtransaction (757)
...
        ReorderBufferAddNewTupleCids(xlrec->top_xid, lsn)
            ReorderBufferTXNByXid()
                -> A ReorderBufferTXN for top-transaciton (756) was generated.
                   The first_lsn was the head of NEW_CID record.
...
DecodeHeapOp(info = XLOG_HEAP_INSERT)
    SnapBuildProcessChange(xid)
        ReorderBufferSetBaseSnapshot(xid, lsn)
            AssertTXNLsnOrder()
                -> A subtransaction was found and it was not an empty transaction.
                -> Next entry was a top-transaction.
                   The previous entry was not empty, and they had same first_lsn. It caused an Assertion failure.
```

## How to fix

I think the straightforward fix is to associate them to top-sub relationship,
and attached patch did it.

Thought?

[1]:
https://www.postgresql.org/message-id/TYCPR01MB12077573479C5A2471BDE8065F5232%40TYCPR01MB12077.jpnprd01.prod.outlook.com
[2]: https://www.postgresql.org/message-id/18369-ad61699bf91c5bc0%40postgresql.org

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/

On Wed, Mar 6, 2024 at 9:04 AM ocean_li_996 <ocean_li_996@163.com> wrote:
>
> Thanks for your attention.
>
> At 2024-03-05 17:24:05, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> wrote:
>
> >Dear Haiyang Li,
> >
> >...
> >## Found issue
> >
> >### Empty transaction is decoded on PG14 and PG15
> >
> >However, there is a room for generating ReorderBufferTxn for empty transactions,
> >which was introduced by 6b77048e5. Conditions are:
> >
> >1. There are sub transactions which modify only temp tables, and
> >2. the top transaction modifies the catalog.
> >
> >The call-stack toward the generation is below.
> >
> >```
> >ReorderBufferTXNByXid(create = true, create_as_top = true)
> >ReorderBufferXidSetCatalogChanges()    // for sub transactions
> >SnapBuildXidSetCatalogChanges()          // for top transaction
> >DecodeCommit()                                      // for top transaction
> >```
> >
> >The path has been introduced by 6b77048e5.
> >Previously, calling ReorderBufferXidSetCatalogChanges() for sub transactions
> >would be skipped, if they do not have catalog changes or they have not decoded yet.
> >However, the commit ensures sub transactions must be marked as containing
> >catalog changes, and this also enforces to decode transactions even if it is
> >empty.
> >
> >### Assertion failure
> >
> >The empty transactions would be created as top transactions. At that time,
> >AssertTXNLsnOrder() is called so that we ensured that first_lsn of top-transactions
> >must be strictly higher than previous. But they can be the same if there are more
> >than two empty transactions. It led an assertion failure.
> >
> Your analysis is correct for me.  Actually, I mentioned in [1] that I can reproduce this issue before 6b77048e5.
> After some attempts and analysis, I also believe that the issue will only occur after 6b77048e5.
>
> > ...
> >
> >## Possible solutions
> >
> >I think there are several solutions.
> >Note that I assumed here that fixes for all the versions should be almost the same.
> >
> >* Ease the condition in AssertTXNLsnOrder(). If the decoded transaction is empty,
> >  it can be allowed that the first_lsn is same as previous one.
> >  PSA file to see my consideration.
> LGFM.  For my observation, the most case failed on AsserTXNOrder is checking empty
> decoded transaction. Maybe we should pay more attention to review ReorderBufferTXNIsEmpty.
>
> >* Generate a ReorderBufferTXN as sub transaction when we are in this path.
> >  The approach has already been shared by you. However, note that this needs to
> >  extend the ReorderBufferXidSetCatalogChanges function, and breaks ABI
> >  compatibility [1].
> Yes, It breaks ABI compatibility.
>

One possibility is introducing ReorderBufferXidSetCatalogChangesEx, a
new API with a bool parameter. I don't know if this is a good idea but
I prefer not to tinker with asserts as proposed by Kuroda-San in
another approach.

--
With Regards,
Amit Kapila.

Dear Amit, 

OK, so let's consider the approach. Note that at this stage, another failure [1]
was ignored.

Here are patches for all supported versions. The failure on PG12-PG15 has been
fixed on my env, but you can confirm as well.

The approach is almost same as what initially shared by Haiyang [2]. However,
instead of extending ReorderBufferXidSetCatalogChanges(), a new function
ReorderBufferXidSetCatalogChangesEx() was added.

Note again that there were changes also in ReorderBufferAssignChild() and
ReorderBufferCleanupTXN(). Extended ReorderBufferXidSetCatalogChanges would the
create ReorderBufferTXN not as top, however, these transaction would not be
associated with the top one. So there is a possibility that txn->node is invalid.
IIUC, only ReorderBufferAssignChild() calls ReorderBufferTXNByXid with create = true
and create_as_top = false, and they would be immediately associated in below.

```
    /* add to subtransaction list */
    dlist_push_tail(&txn->subtxns, &subtxn->node);
    txn->nsubtxns++;
```

[1]:
https://www.postgresql.org/message-id/TYCPR01MB12077573479C5A2471BDE8065F5232%40TYCPR01MB12077.jpnprd01.prod.outlook.com
[2]: https://www.postgresql.org/message-id/6d0e80d6.c1fc.18deeb8120a.Coremail.ocean_li_996%40163.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

Dear Haiyang,

Thanks for checking! This reply was still focused only on "Issue 2" in your notation.

>## Issue 2
>Inspired by your spec case, I've reorganized the spec case provided in [2]. The new test in attachment
>is  able to reproduce the issue mentioned in [1] even before commit 6b77048e5.

Good findings. I also confirmed the workload could fail after reverting the 6b77048e5.
Also confirmed that the patch [1] could fix the workload as well.

permutation "s0_init" "s0_begin" "s0_savepoint" "s0_create_part1" "s0_savepoint_release"
                     "s2_init" "s1_checkpoint" "s1_get_changes" "s0_commit" "s2_get_changes"

## Analysis

The point was that the serialized snapshot by another replication slot can be reused.
When the first get_change is called, a consistent snapshot can be serialized because
of the XLOG_RUNNING_XACTS record (see later).
The get_changes for the second slot reuses so that it can read WAL records property.
(If the first slot does not exist, the status of the snapshot would be
SNAPBUILD_BUILDING_SNAPSHOT. So no records are read)

In the second get_changes, below records are read. First (LOCK, RUNNING_XACTS)
pair is generated from the slot creation, and second pair comes from the
CHECKPOINT. I.e., it reads all records from the slot generation.

```
...lsn: 0/01906DB8, prev 0/01906D58, desc: LOCK ...
...lsn: 0/01906DF0, prev 0/01906DB8, desc: RUNNING_XACTS ...
...lsn: 0/01906E30, prev 0/01906DF0, desc: LOCK ...
...lsn: 0/01906E68, prev 0/01906E30, desc: RUNNING_XACTS ...
...lsn: 0/01906EA8, prev 0/01906E68, desc: CHECKPOINT_ONLINE ...
...lsn: 0/01906F20, prev 0/01906EA8, desc: COMMIT ... subxacts: 728; ... inval msgs: ...
```

Also the final COMMIT record contains the info for a subtransaction and
XACT_XINFO_HAS_INVALS flag, so DecodeCommit()->SnapBuildXidSetCatalogChanges()
is called transactions.

After that, two ReorderBufferTXNs are created with the same LSN, it fails the
assertion in AssertTXNLsnOrder().

I will update the patch if above analysis is correct.

>The approach in [3] is also LGFM.

Thanks. I agreed that we should not ease condition for Assert() as much as possible.

[1]:
https://www.postgresql.org/message-id/TYCPR01MB1207790E98F0A563280CC39FCF5262%40TYCPR01MB12077.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/

Dear Haiyang Li,

> Your analysis is correct for me.

Thanks for the confirmation. Here are updated patch for REL12-REL15.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/

Dear Fei,

Thanks for giving a comment!

>Is it better for ReorderBufferXidSetCatalogChanges to directly call the
>ReorderBufferXidSetCatalogChangesEx function? , which can reduce the cost of
>later maintenance.

Indeed, PSA fixed version.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/

Dear hackers,

Regarding the second failure, which was reported the upstream.

While reading some other threads, I found that the similar phenomenon has already
been reported in [1]. The primal reason was that slots reused the serialized
snapshot but it was not correct. Therefore, I thought the second failure should
be discussed on the referred thread and here we should focused on the recently
proposed fix by me.
(This meant that my proposed workload may not work well after the fix. Will modify
the test later if needed.)

[1]: https://www.postgresql.org/message-id/flat/2444AA15-D21B-4CCE-8052-52C7C2DAFE5C%40amazon.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/

Dear Haiyang Li,

> Issue #1 is presented in [1], which has been figured out and provided an appropriate fix patch in [5].

Right.

> Issue #2 is presented in [2], which has been figured out. But we have not yet provided a fix patch.
> Issue #3 is presented in [3], which was initially thought to be the same as issue #2. However,
> you have found that it might be related to the issue discussed in [4].

Sorry for confusing. I meant to say that issues #2 and #3 are the same, and there is a related
discussion in [4].

> If my understanding is correct, I think that multiple issues should be resolved independently.
> In this thread, we should resolve issue #1 using the patch provided in [5]. If we want to prevent
> the newly added spec test triggering other issues (such as issue #3), revert the spec test to
> the test cases provided in [6] may be a good choice.

Absolutely correct.

> The issue  #2 can be discussed in a new thread. The issue #3 can be discussed in [4] thread.
> Thought?

Again, I think #2 and #3 are the same, so it should be discussed in [4].

[4]: https://www.postgresql.org/message-id/29273AF3-9684-4069-9257-D05090B03B99@amazon.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/

On Wed, Mar 13, 2024 at 3:24 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Regarding the second failure, which was reported the upstream.
>
> While reading some other threads, I found that the similar phenomenon has already
> been reported in [1]. The primal reason was that slots reused the serialized
> snapshot but it was not correct. Therefore, I thought the second failure should
> be discussed on the referred thread and here we should focused on the recently
> proposed fix by me.
> (This meant that my proposed workload may not work well after the fix. Will modify
> the test later if needed.)
>

I agree with this. Is there a reason you are not using the test for
the issue originally reported in this thread?

--
With Regards,
Amit Kapila.

On Fri, Mar 15, 2024 at 6:16 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Mar 13, 2024 at 3:24 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Regarding the second failure, which was reported the upstream.
> >
> > While reading some other threads, I found that the similar phenomenon has already
> > been reported in [1]. The primal reason was that slots reused the serialized
> > snapshot but it was not correct. Therefore, I thought the second failure should
> > be discussed on the referred thread and here we should focused on the recently
> > proposed fix by me.
> > (This meant that my proposed workload may not work well after the fix. Will modify
> > the test later if needed.)
> >
>
> I agree with this. Is there a reason you are not using the test for
> the issue originally reported in this thread?
>

To verify my theory, I tried by slight modification in the test
proposed by your patch:
proposed:
"s0_init" "s0_begin" "s0_savepoint" "s0_create_part1"
"s0_savepoint_release" "s2_init" "s1_checkpoint" "s1_get_changes"
"s0_commit" "s2_get_changes" "s2_stop"

changed:
"s0_init" "s0_begin" "s0_savepoint" "s0_create_part1"
"s0_savepoint_release" "s2_init" "s1_checkpoint" "s1_get_changes"
"so_insert_part1" "s0_commit" "s2_get_changes" "s2_stop"

Note that so_insert_part1 (define it as: Insert into tbl1_part
values(2);) before s0_commit and I get the error: "ERROR:  could not
map filenode "base/5/16387" to relation OID". So, I feel such a test
case needs the fix being discussed in the thread [1]. I don't know
whether there will be any relation between the fixes for both the
issues but it seems better to evaluate the other one [1] as well
before moving forward with the fix in this thread.

[1] - https://www.postgresql.org/message-id/29273AF3-9684-4069-9257-D05090B03B99@amazon.com

--
With Regards,
Amit Kapila.

Dear Amit,

> I agree with this. Is there a reason you are not using the test for
> the issue originally reported in this thread?

Firstly the modified version was used to shorten the test workload.
However, the optimization assumed the BUG which may be fixed in future.
Here are new patch set which recent changes for testcases were reverted.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

On Thu, May 16, 2024 at 4:23 PM ocean_li_996 <ocean_li_996@163.com> wrote:
>
> Hi all:
>
>
> Two months have passed by now, and I'm wondering if the issue mentioned in [1] has been resolved yet?

I've pushed the fix, bb19b70081.

>
> Returning to this question, does anyone have any new opinions on it? If not, should we consider fixing this problem?
>
> Or is this considered a bug that's not worth fixing?

I think we should fix this bug too. I've reviewed the proposed v4
patch for pg15 and here are comments:

---
+# DML operations on temporary table temp_t consume XIDs, but do not generate
+# WAL records. The last decoding restarts from the first checkpoint and doesn't
+# get any information of last two substransactions that performed
s0_insert_temp.
+# While processing the commit record for the corresponding top-level
+# transaction which will be marked as containing catalog change even before
+# commit, we ensure that the corresponding substransaction is added into
+# ReorderBuffer as subxact.
+permutation "s0_create_temp" "s0_init" "s0_begin" "s0_txid_current"
"s1_checkpoint" "s0_savepoint" "s0_insert_temp" "s0_savepoint_release"
"s0_savepoint" "s0_insert_temp" "s0_savepoint_release" "s0_savepoint"
"s0_insert_temp" "s0_savepoint_release" "s0_create_part1"
"s1_get_changes" "s0_commit" "s1_get_changes"

I think the new test case can be simplified to something like:

permutation "s0_create_temp" "s0_init" "s0_begin" "s0_txid_current"
"s1_checkpoint" "s0_truncate" "s0_savepoint" "s0_insert_temp"
"s0_savepoint" "s0_insert_temp" "s0_savepoint" "s0_insert_temp"
"s1_get_changes" "s0_commit" "s1_get_changes"

It has fewer steps and emits fewer WAL records.

---
+                        *
+                        * Note: There is a possibility that the
transaction was created
+                        * as not the top-level txn, but the flag was
not set. In this
+                        * case, the transaction was not added to the
list. This happens if
+                        * sub-txns are first recognized and decoded
by a COMMIT record.
                         */
-                       dlist_delete(&subtxn->node);
+                       if (subtxn->node.next != NULL)
+                               dlist_delete(&subtxn->node);
                }

I think that the new if statement can be merged to the above else
block (i.e, "else if (subtx->node.next != NULL)").

It's not clear to me what the "the flag" in the new comment refers to.
How about rephrasing it to:

/*
 * We already saw this transaction, but might have initially added
 * it to the list of top-level txns.  Now that we know it's not
 * top-level, remove it from there if it's already attached to the
 * list.
 */

---
        /*
-        * Remove TXN from its containing list.
+        * Remove TXN from its containing list if registered.
         *

How about "if attached"? On HEAD, we use the terms "detached" for
dlist_node (see dlist_node_is_detached() in ilist.h).

---
+void
+ReorderBufferXidSetCatalogChangesEx(ReorderBuffer *rb, TransactionId xid,
+                                   XLogRecPtr lsn, bool is_top)
 {
    ReorderBufferTXN *txn;

-   txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true);
+   txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, is_top);

and

 -           ReorderBufferXidSetCatalogChanges(builder->reorder,
subxacts[i], lsn);
 +           ReorderBufferXidSetCatalogChangesEx(builder->reorder,
subxacts[i], lsn, false);

As Kuroda-san previously mentioned[1], with this change, we would end
up with a transaction entry that is not the top-level transaction but
is not assigned to any top-level transaction. Such transactions are
created in SnapBuildXidSetCatalogChanges(), used solely for reference
by SnapBuildCommitTxn(), and promptly cleaned up afterward. Therefore
I don't believe it would pose a significant issue, but I will
contemplate it further. I feel it's somewhat of a hacky solution just
to avoid AssertTXNLsnOrder().

In SnapBuildXidSetCatalogChanges(), since we know that all XIDs in the
subxacts array are subtransactions of the transaction 'xid', another
way to fix it would be to properly assign all subtransactions to the
top-level transaction (i.e., by calling ReorderBufferAssignChild())
before setting each of them as catalog-changes. However, this change
alone is insufficient, as it has been possible that we accumulate
invalidation messages into a transaction that will later be identified
as a subtransaction. IOW, another assertion in ReorderBufferForget()
would fail:

    /*
     * Process cache invalidation messages if there are any. Even if we're not
     * interested in the transaction's contents, it could have manipulated the
     * catalog and we need to update the caches according to that.
     */
    if (txn->base_snapshot != NULL && txn->ninvalidations > 0)
        ReorderBufferImmediateInvalidation(rb, txn->ninvalidations,
                                           txn->invalidations);
    else
        Assert(txn->ninvalidations == 0);

Note that when assigning a subtransaction to its top-level
transaction, we transfer the subtransaction's snapshot to its
top-level transaction but not for the invalidation messages in
ReorderBufferTXN. Consequently, it's possible to have subtransactions
that lack the base snapshot but possess invalidation messages.
Therefore, we would need to check if txn->ninvalidations == 0 only for
top-level transactions. That is:

-   else
+   else if (!rbtxn_is_known_subxact(txn))
        Assert(txn->ninvalidations == 0);

I'm uncertain which approach is better or if there exists an even
superior approach. I'll continue contemplating it. Feedback is highly
welcomed.

Regards,

[1]
https://www.postgresql.org/message-id/TYCPR01MB1207790E98F0A563280CC39FCF5262%40TYCPR01MB12077.jpnprd01.prod.outlook.com

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com