[BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns
Sending this to pgsql-hackers list to create a CommitFest entry with the attached patch proposal.
Hello,
We noticed that the logical replication could fail when the Standby::RUNNING_XACT record is generated in the middle of a catalog modifying transaction and if the logical decoding has to restart from the RUNNING_XACT
WAL entry.
The Standby::RUNNING_XACT record is generated periodically (roughly every 15s by default) or during a CHECKPOINT operation.
Detailed problem description:
Tested on 11.8 & current master.
The logical replication slot restart_lsn advances in the middle of an open txn that modified the catalog (e.g. TRUNCATE operation).
Should the logical decoding has to restart it could fail with an error like this:
ERROR: could not map filenode "base/13237/442428"
Currently, the system relies on processing Heap2::NEW_CID to keep track of catalog modifying (sub)transactions.
This context is lost if the logical decoding has to restart from a Standby::RUNNING_XACTS that is written between the NEW_CID record and its parent txn commit.
If the logical stream restarts from this restart_lsn, then it doesn't have the xid responsible for modifying the catalog.
Repro steps:
1. We need to generate the Standby::RUNNING_XACT record deterministically using CHECKPOINT. Hence we'll delay the LOG_SNAPSHOT_INTERVAL_MS using the following patch:
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c
index 3e6ffb05b9..b776e8d566 100644
--- a/src/backend/postmaster/bgwriter.c
+++ b/src/backend/postmaster/bgwriter.c
@@ -76,7 +76,7 @@ int BgWriterDelay = 200;* Interval in which standby snapshots are logged into the WAL stream, in
* milliseconds.
/
-#define LOG_SNAPSHOT_INTERVAL_MS 15000
+#define LOG_SNAPSHOT_INTERVAL_MS 1500000
2. Create a table
postgres=# create table bdt (a int);
CREATE TABLE
3. Create a logical replication slot:
postgres=# select pg_create_logical_replication_slot('bdt_slot','test_decoding');
pg_create_logical_replication_slot
------------------------------------
(bdt_slot,0/FFAA1C70)
(1 row)
4. Start reading the slot in a shell (keep the shell so that we can stop reading later):
./bin/pg_recvlogical --slot bdt_slot --start -f bdt.out -d postgres
5. Execute the workload across 2 different clients in the following order
Session1:
begin;
savepoint b1;
truncate bdt;Session2:
select * from pg_replication_slots; /* keep note of the confirmed_flush_lsn */
checkpoint;
/* Repeat the following query until the confirmed_flush_lsn changes */
select * from pg_replication_slots;
Once confirmed_flush_lsn, changes:
Session1:
end;
begin;
insert into bdt values (1);
Session2:
select * from pg_replication_slots; /* keep note of both restart_lsn AND the confirmed_flush_lsn */
checkpoint;
/* Repeat the following query until both restart_lsn AND confirmed_flush_lsn change */
select * from pg_replication_slots;
6. Stop the pg_recvlogical (Control-C)
7. Then commit the insert txn:
Session1:
end;
8. Get/peek the replication slot changes
postgres=# select * from pg_logical_slot_get_changes('bdt_slot', null, null);
ERROR: could not map filenode "base/13237/442428" to relation OID
Proposed solution:
If we’re decoding a catalog modifying commit record, then check whether it’s part of the RUNNING_XACT xid’s processed @ the restart_lsn. If so, then add its xid & subxacts in the committed txns list in the logical decoding snapshot.
Please refer to the attachment for the proposed patch.
Thanks,
Mike
Attachments:
logical_decoding_xact_bookkeep.patchapplication/octet-stream; name=logical_decoding_xact_bookkeep.patchDownload+62-8
Import Notes
Reply to msg id not found: 60230E84-4201-46E4-9AF1-36AD3674BA88@amazon.comReference msg id not found: 60230E84-4201-46E4-9AF1-36AD3674BA88@amazon.com
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested
I have also seen this error with logical replication using pglogical extension, will this patch also address similar problem with pglogical?
On Fri, 07 May 2021 at 19:50, ahsan hadi <ahsan.hadi@gmail.com> wrote:
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not testedI have also seen this error with logical replication using pglogical extension, will this patch also address similar problem with pglogical?
Does there is a test case to reproduce this problem (using pglogical)?
I encountered this, however I'm not find a case to reproduce it.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On Sat, May 8, 2021 at 8:17 AM Japin Li <japinli@hotmail.com> wrote:
On Fri, 07 May 2021 at 19:50, ahsan hadi <ahsan.hadi@gmail.com> wrote:
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not testedI have also seen this error with logical replication using pglogical
extension, will this patch also address similar problem with pglogical?
Does there is a test case to reproduce this problem (using pglogical)?
I encountered this, however I'm not find a case to reproduce it.
I have seen a user run into this with pglogical, the error is produced
after logical decoding finds an inconsistent point. However we haven't been
able to reproduce the user scenario locally...
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
--
Highgo Software (Canada/China/Pakistan)
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
EMAIL: mailto: ahsan.hadi@highgo.ca
This patch should address the same problem for pglogical as well.
Thanks,
Mike
On 6/4/21, 3:55 PM, "ahsan hadi" <ahsan.hadi@gmail.com> wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested
I have also seen this error with logical replication using pglogical extension, will this patch also address similar problem with pglogical?
On Tue, Mar 16, 2021 at 1:35 AM Oh, Mike <minsoo@amazon.com> wrote:
Sending this to pgsql-hackers list to create a CommitFest entry with the attached patch proposal.
Hello,
We noticed that the logical replication could fail when the Standby::RUNNING_XACT record is generated in the middle of a catalog modifying transaction and if the logical decoding has to restart from the RUNNING_XACT
WAL entry.
The Standby::RUNNING_XACT record is generated periodically (roughly every 15s by default) or during a CHECKPOINT operation.
Detailed problem description:
Tested on 11.8 & current master.
The logical replication slot restart_lsn advances in the middle of an open txn that modified the catalog (e.g. TRUNCATE operation).
Should the logical decoding has to restart it could fail with an error like this:
ERROR: could not map filenode "base/13237/442428"
Thank you for reporting the issue.
I could reproduce this issue by the steps you shared.
Currently, the system relies on processing Heap2::NEW_CID to keep track of catalog modifying (sub)transactions.
This context is lost if the logical decoding has to restart from a Standby::RUNNING_XACTS that is written between the NEW_CID record and its parent txn commit.
If the logical stream restarts from this restart_lsn, then it doesn't have the xid responsible for modifying the catalog.
I agree with your analysis. Since we don’t use commit WAL record to
track the transaction that has modified system catalogs, if we decode
only the commit record of such transaction, we cannot know the
transaction has been modified system catalogs, resulting in the
subsequent transaction scans system catalog with the wrong snapshot.
With the patch, if the commit WAL record has a XACT_XINFO_HAS_INVALS
flag and its xid is included in RUNNING_XACT record written at
restart_lsn, we forcibly add the top XID and its sub XIDs as a
committed transaction that has modified system catalogs to the
snapshot. I might be missing something about your patch but I have
some comments on this approach:
1. Commit WAL record may not have invalidation message for system
catalogs (e.g., when commit record has only invalidation message for
relcache) even if it has XACT_XINFO_HAS_INVALS flag. In this case, the
transaction wrongly is added to the snapshot, is that okay?
2. We might add a subtransaction XID as a committed transaction that
has modified system catalogs even if it actually didn't. As the
comment in SnapBuildBuildSnapshot() describes, we track only the
transactions that have modified the system catalog and store in the
snapshot (in the ‘xip' array). The patch could break that assumption.
However, I’m really not sure how to deal with this point. We cannot
know which subtransaction has actually modified system catalogs by
using only the commit WAL record.
3. The patch covers only the case where the restart_lsn exactly
matches the LSN of RUNNING_XACT. I wonder if there could be a case
where the decoding starts at a WAL record other than RUNNING_XACT but
the next WAL record is RUNNING_XACT.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
Hi,
On 7/29/21 10:25 AM, Masahiko Sawada wrote:
Thank you for reporting the issue.
I could reproduce this issue by the steps you shared.
Thanks for looking at it!
Currently, the system relies on processing Heap2::NEW_CID to keep track of catalog modifying (sub)transactions.
This context is lost if the logical decoding has to restart from a Standby::RUNNING_XACTS that is written between the NEW_CID record and its parent txn commit.
If the logical stream restarts from this restart_lsn, then it doesn't have the xid responsible for modifying the catalog.
I agree with your analysis. Since we don’t use commit WAL record to
track the transaction that has modified system catalogs, if we decode
only the commit record of such transaction, we cannot know the
transaction has been modified system catalogs, resulting in the
subsequent transaction scans system catalog with the wrong snapshot.
Right.
With the patch, if the commit WAL record has a XACT_XINFO_HAS_INVALS
flag and its xid is included in RUNNING_XACT record written at
restart_lsn, we forcibly add the top XID and its sub XIDs as a
committed transaction that has modified system catalogs to the
snapshot. I might be missing something about your patch but I have
some comments on this approach:1. Commit WAL record may not have invalidation message for system
catalogs (e.g., when commit record has only invalidation message for
relcache) even if it has XACT_XINFO_HAS_INVALS flag.
Right, good point (create policy for example would lead to an
invalidation for relcache only).
In this case, the
transaction wrongly is added to the snapshot, is that okay?
This transaction is a committed one, and IIUC the snapshot would be used
only for catalog visibility, so i don't see any issue to add it in the
snapshot, what do you think?
2. We might add a subtransaction XID as a committed transaction that
has modified system catalogs even if it actually didn't.
Right, like when needs_timetravel is true.
As the
comment in SnapBuildBuildSnapshot() describes, we track only the
transactions that have modified the system catalog and store in the
snapshot (in the ‘xip' array). The patch could break that assumption.
Right. It looks to me that breaking this assumption is not an issue.
IIUC currently the committed ones that are not modifying the catalog are
not stored "just" because we don't need them.
However, I’m really not sure how to deal with this point. We cannot
know which subtransaction has actually modified system catalogs by
using only the commit WAL record.
Right, unless we rewrite this patch so that a commit WAL record will
produce this information.
3. The patch covers only the case where the restart_lsn exactly
matches the LSN of RUNNING_XACT.
Right.
I wonder if there could be a case
where the decoding starts at a WAL record other than RUNNING_XACT but
the next WAL record is RUNNING_XACT.
Not sure, but could a restart_lsn not be a RUNNING_XACTS?
Thanks
Bertrand
Hi
On Tuesday, March 16, 2021 1:35 AM Oh, Mike <minsoo@amazon.com> wrote:
We noticed that the logical replication could fail when the
Standby::RUNNING_XACT record is generated in the middle of a catalog
modifying transaction and if the logical decoding has to restart from the
RUNNING_XACT
WAL entry.
...
Proposed solution:
If we’re decoding a catalog modifying commit record, then check whether
it’s part of the RUNNING_XACT xid’s processed @ the restart_lsn. If so,
then add its xid & subxacts in the committed txns list in the logical decoding
snapshot.Please refer to the attachment for the proposed patch.
Let me share some review comments for the patch.
(1) last_running declaration
Isn't it better to add static for this variable,
because we don't use this in other places ?
@@ -85,6 +86,9 @@ static bool DecodeTXNNeedSkip(LogicalDecodingContext *ctx,
XLogRecordBuffer *buf, Oid dbId,
RepOriginId origin_id);
+/* record previous restart_lsn running xacts */
+xl_running_xacts *last_running = NULL;(2) DecodeStandbyOp's memory free
I'm not sure when
we pass this condition with already allocated last_running,
but do you need to free it's xid array here as well,
if last_running isn't null ?
Otherwise, we'll miss the chance after this.
+ /* record restart_lsn running xacts */
+ if (MyReplicationSlot && (buf->origptr == MyReplicationSlot->data.restart_lsn))
+ {
+ if (last_running)
+ free(last_running);
+
+ last_running = NULL;(3) suggestion of small readability improvement
We calculate the same size twice here and DecodeCommit.
I suggest you declare a variable that stores the computed result of size,
which might shorten those codes.
+ /*
+ * xl_running_xacts contains a xids Flexible Array
+ * and its size is subxcnt + xcnt.
+ * Take that into account while allocating
+ * the memory for last_running.
+ */
+ last_running = (xl_running_xacts *) malloc(sizeof(xl_running_xacts)
+ + sizeof(TransactionId )
+ * (running->subxcnt + running->xcnt));
+ memcpy(last_running, running, sizeof(xl_running_xacts)
+ + (sizeof(TransactionId)
+ * (running->subxcnt + running->xcnt)));Best Regards,
Takamichi Osumi
On 7/29/21 01:25, Masahiko Sawada wrote:
On Tue, Mar 16, 2021 at 1:35 AM Oh, Mike <minsoo@amazon.com> wrote:
Sending this to pgsql-hackers list to create a CommitFest entry with the attached patch proposal.
...
Detailed problem description:
Tested on 11.8 & current master.
The logical replication slot restart_lsn advances in the middle of an open txn that modified the catalog (e.g. TRUNCATE operation).
Should the logical decoding has to restart it could fail with an error like this:
ERROR: could not map filenode "base/13237/442428"
Thank you for reporting the issue.
I could reproduce this issue by the steps you shared.
I also noticed a bug report earlier this year with another PG user
reporting the same error - on version 12.3
/messages/by-id/16812-3d9df99bd77ff616@postgresql.org
Today I received a report from a new PG user of this same error message
causing their logical replication to break. This customer was also
running PostgreSQL 12.3 on both source and target side.
Haven't yet dumped WAL or anything, but wanted to point out that the
error is being seen in the wild - I hope we can get a version of this
patch committed soon, as it will help with at least one cause.
-Jeremy
--
Jeremy Schneider
Database Engineer
Amazon Web Services
On Friday, September 24, 2021 5:03 PM I wrote:
On Tuesday, March 16, 2021 1:35 AM Oh, Mike <minsoo@amazon.com> wrote:
We noticed that the logical replication could fail when the
Standby::RUNNING_XACT record is generated in the middle of a catalog
modifying transaction and if the logical decoding has to restart from
the RUNNING_XACT WAL entry....
Proposed solution:
If we’re decoding a catalog modifying commit record, then check
whether it’s part of the RUNNING_XACT xid’s processed @ the
restart_lsn. If so, then add its xid & subxacts in the committed txns
list in the logical decoding snapshot.Please refer to the attachment for the proposed patch.
Let me share some review comments for the patch.
....
(3) suggestion of small readability improvement
We calculate the same size twice here and DecodeCommit.
I suggest you declare a variable that stores the computed result of size, which
might shorten those codes.+ /* + * xl_running_xacts contains a xids Flexible Array + * and its size is subxcnt + xcnt. + * Take that into account while allocating + * the memory for last_running. + */ + last_running = (xl_running_xacts *) malloc(sizeof(xl_running_xacts) + + sizeof(TransactionId ) + * (running->subxcnt + running->xcnt)); + memcpy(last_running, running, sizeof(xl_running_xacts) + + (sizeof(TransactionId) + + * (running->subxcnt + running->xcnt)));
Let me add one more basic review comment in DecodeStandbyOp().
Why do you call raw malloc directly ?
You don't have the basic check whether the return value is
NULL or not and intended to call palloc here instead ?
Best Regards,
Takamichi Osumi
On Thu, Sep 23, 2021 at 5:44 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
Hi,
On 7/29/21 10:25 AM, Masahiko Sawada wrote:
Thank you for reporting the issue.
I could reproduce this issue by the steps you shared.
Thanks for looking at it!
Currently, the system relies on processing Heap2::NEW_CID to keep track of catalog modifying (sub)transactions.
This context is lost if the logical decoding has to restart from a Standby::RUNNING_XACTS that is written between the NEW_CID record and its parent txn commit.
If the logical stream restarts from this restart_lsn, then it doesn't have the xid responsible for modifying the catalog.
I agree with your analysis. Since we don’t use commit WAL record to
track the transaction that has modified system catalogs, if we decode
only the commit record of such transaction, we cannot know the
transaction has been modified system catalogs, resulting in the
subsequent transaction scans system catalog with the wrong snapshot.Right.
With the patch, if the commit WAL record has a XACT_XINFO_HAS_INVALS
flag and its xid is included in RUNNING_XACT record written at
restart_lsn, we forcibly add the top XID and its sub XIDs as a
committed transaction that has modified system catalogs to the
snapshot. I might be missing something about your patch but I have
some comments on this approach:1. Commit WAL record may not have invalidation message for system
catalogs (e.g., when commit record has only invalidation message for
relcache) even if it has XACT_XINFO_HAS_INVALS flag.Right, good point (create policy for example would lead to an
invalidation for relcache only).In this case, the
transaction wrongly is added to the snapshot, is that okay?This transaction is a committed one, and IIUC the snapshot would be used
only for catalog visibility, so i don't see any issue to add it in the
snapshot, what do you think?
It seems to me that it's no problem since we always transaction with
catalog-changed when decoding XLOG_XACT_INVALIDATIONS records.
2. We might add a subtransaction XID as a committed transaction that
has modified system catalogs even if it actually didn't.Right, like when needs_timetravel is true.
As the
comment in SnapBuildBuildSnapshot() describes, we track only the
transactions that have modified the system catalog and store in the
snapshot (in the ‘xip' array). The patch could break that assumption.Right. It looks to me that breaking this assumption is not an issue.
IIUC currently the committed ones that are not modifying the catalog are
not stored "just" because we don't need them.However, I’m really not sure how to deal with this point. We cannot
know which subtransaction has actually modified system catalogs by
using only the commit WAL record.Right, unless we rewrite this patch so that a commit WAL record will
produce this information.3. The patch covers only the case where the restart_lsn exactly
matches the LSN of RUNNING_XACT.Right.
I wonder if there could be a case
where the decoding starts at a WAL record other than RUNNING_XACT but
the next WAL record is RUNNING_XACT.Not sure, but could a restart_lsn not be a RUNNING_XACTS?
I guess the decoding always starts from RUNING_XACTS.
After more thought, I think that the basic approach of the proposed
patch is a probably good idea, which we add xid whose commit record
has XACT_XINFO_HAS_INVALS to the snapshot. The problem as I see is
that during decoding COMMIT record we cannot know which transactions
(top transaction or subtransactions) actually did catalog changes. But
given that even if XLOG_XACT_INVALIDATION has only relcache
invalidation message we always mark the transaction with
catalog-changed, it seems no problem. Therefore, in the reported
cases, probably we can add both the top transaction xid and its
subscription xids to the snapshot.
Regarding the patch details, I have two comments:
---
+ if ((parsed->xinfo & XACT_XINFO_HAS_INVALS) && last_running)
+ {
+ /* make last_running->xids bsearch()able */
+ qsort(last_running->xids,
+ last_running->subxcnt + last_running->xcnt,
+ sizeof(TransactionId), xidComparator);The patch does qsort() every time when the commit message has
XACT_XINFO_HAS_INVALS. IIUC the xids we need to remember is the only
xids that are recorded in the first replayed XLOG_RUNNING_XACTS,
right? If so, we need to do qsort() once, can remove xid from the
array once it gets committed, and then can eventually make
last_running empty so that we can skip even TransactionIdInArray().
---
Since last_running is allocated by malloc() and it isn't freed even
after finishing logical decoding.
Another idea to fix this problem would be that before calling
SnapBuildCommitTxn() we create transaction entries in ReorderBuffer
for (sub)transactions whose COMMIT record has XACT_XINFO_HAS_INVALS,
and then mark all of them as catalog-changed by calling
ReorderBufferXidSetCatalogChanges(). I've attached a PoC patch for
this idea. What the patch does is essentially the same as what the
proposed patch does. But the patch doesn't modify the
SnapBuildCommitTxn(). And we remember the list of last running
transactions in reorder buffer and the list is periodically purged
during decoding RUNNING_XACTS records, eventually making it empty.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
Attachments:
poc_remember_last_running_xacts.patchapplication/octet-stream; name=poc_remember_last_running_xacts.patchDownload+129-0
On Thursday, October 7, 2021 1:20 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Regarding the patch details, I have two comments:
--- + if ((parsed->xinfo & XACT_XINFO_HAS_INVALS) && last_running) { + /* make last_running->xids bsearch()able */ + qsort(last_running->xids, + last_running->subxcnt + last_running->xcnt, + sizeof(TransactionId), xidComparator);The patch does qsort() every time when the commit message has
XACT_XINFO_HAS_INVALS. IIUC the xids we need to remember is the only
xids that are recorded in the first replayed XLOG_RUNNING_XACTS, right? If so,
we need to do qsort() once, can remove xid from the array once it gets
committed, and then can eventually make last_running empty so that we can
skip even TransactionIdInArray().---
Since last_running is allocated by malloc() and it isn't freed even after finishing
logical decoding.Another idea to fix this problem would be that before calling
SnapBuildCommitTxn() we create transaction entries in ReorderBuffer for
(sub)transactions whose COMMIT record has XACT_XINFO_HAS_INVALS,
and then mark all of them as catalog-changed by calling
ReorderBufferXidSetCatalogChanges(). I've attached a PoC patch for this idea.
What the patch does is essentially the same as what the proposed patch does.
But the patch doesn't modify the SnapBuildCommitTxn(). And we remember
the list of last running transactions in reorder buffer and the list is periodically
purged during decoding RUNNING_XACTS records, eventually making it
empty.
Thanks for the patch.
Conducted a quick check of the POC.
Test of check-world PASSED with your patch and head.
Also, the original scenario described in [1]/messages/by-id/81D0D8B0-E7C4-4999-B616-1E5004DBDCD2@amazon.com looks fine
with your revised patch and LOG_SNAPSHOT_INTERVAL_MS expansion in the procedure.
The last command in the provided steps showed below.
postgres=# select * from pg_logical_slot_get_changes('bdt_slot', null, null);
lsn | xid | data
-----------+-----+----------------------------------------
0/1560020 | 710 | BEGIN 710
0/1560020 | 710 | table public.bdt: INSERT: a[integer]:1
0/1560140 | 710 | COMMIT 710
Minor comments for DecodeStandbyOp changes I noticed instantly
(1) minor suggestion of your comment.
+ * has done catalog changes without these records, we miss to add
+ * the xid to the snapshot so up creating the wrong snapshot. To"miss to add" would be miss adding or fail to add.
And "up creating" is natural in this sentence ?
(2) a full-width space between "it'" and "s" in the next sentence.
+ * mark an xid that actually has not done that but it’s not a
[1]: /messages/by-id/81D0D8B0-E7C4-4999-B616-1E5004DBDCD2@amazon.com
Best Regards,
Takamichi Osumi
At Thu, 7 Oct 2021 13:20:14 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
Another idea to fix this problem would be that before calling
SnapBuildCommitTxn() we create transaction entries in ReorderBuffer
for (sub)transactions whose COMMIT record has XACT_XINFO_HAS_INVALS,
and then mark all of them as catalog-changed by calling
ReorderBufferXidSetCatalogChanges(). I've attached a PoC patch for
this idea. What the patch does is essentially the same as what the
proposed patch does. But the patch doesn't modify the
SnapBuildCommitTxn(). And we remember the list of last running
transactions in reorder buffer and the list is periodically purged
during decoding RUNNING_XACTS records, eventually making it empty.
I came up with the third way. SnapBuildCommitTxn already properly
handles the case where a ReorderBufferTXN with
RBTXN_HAS_CATALOG_CHANGES. So this issue can be resolved by create
such ReorderBufferTXNs in SnapBuildProcessRunningXacts.
One problem with this is that change creates the case where multiple
ReorderBufferTXNs share the same first_lsn. I haven't come up with a
clean idea to avoid relaxing the restriction of AssertTXNLsnOrder..
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
register_running_xacts_at_logical_decoding_start_PoC.txttext/plain; charset=us-asciiDownload+21-2
.
On Fri, Oct 8, 2021 at 4:50 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
At Thu, 7 Oct 2021 13:20:14 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
Another idea to fix this problem would be that before calling
SnapBuildCommitTxn() we create transaction entries in ReorderBuffer
for (sub)transactions whose COMMIT record has XACT_XINFO_HAS_INVALS,
and then mark all of them as catalog-changed by calling
ReorderBufferXidSetCatalogChanges(). I've attached a PoC patch for
this idea. What the patch does is essentially the same as what the
proposed patch does. But the patch doesn't modify the
SnapBuildCommitTxn(). And we remember the list of last running
transactions in reorder buffer and the list is periodically purged
during decoding RUNNING_XACTS records, eventually making it empty.I came up with the third way. SnapBuildCommitTxn already properly
handles the case where a ReorderBufferTXN with
RBTXN_HAS_CATALOG_CHANGES. So this issue can be resolved by create
such ReorderBufferTXNs in SnapBuildProcessRunningXacts.
Thank you for the idea and patch!
It's much simpler than mine. I think that creating an entry of a
catalog-changed transaction in the reorder buffer before
SunapBuildCommitTxn() is the right direction.
After more thought, given DDLs are not likely to happen than DML in
practice, probably we can always mark both the top transaction and its
subtransactions as containing catalog changes if the commit record has
XACT_XINFO_HAS_INVALS? I believe this is not likely to lead to
overhead in practice. That way, the patch could be more simple and
doesn't need the change of AssertTXNLsnOrder().
I've attached another PoC patch. Also, I've added the tests for this
issue in test_decoding.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
Attachments:
poc_mark_all_txns_catalog_change.patchapplication/octet-stream; name=poc_mark_all_txns_catalog_change.patchDownload+96-1
Hi,
On 10/11/21 8:27 AM, Masahiko Sawada wrote:
On Fri, Oct 8, 2021 at 4:50 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:At Thu, 7 Oct 2021 13:20:14 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
Another idea to fix this problem would be that before calling
SnapBuildCommitTxn() we create transaction entries in ReorderBuffer
for (sub)transactions whose COMMIT record has XACT_XINFO_HAS_INVALS,
and then mark all of them as catalog-changed by calling
ReorderBufferXidSetCatalogChanges(). I've attached a PoC patch for
this idea. What the patch does is essentially the same as what the
proposed patch does. But the patch doesn't modify the
SnapBuildCommitTxn(). And we remember the list of last running
transactions in reorder buffer and the list is periodically purged
during decoding RUNNING_XACTS records, eventually making it empty.I came up with the third way. SnapBuildCommitTxn already properly
handles the case where a ReorderBufferTXN with
RBTXN_HAS_CATALOG_CHANGES. So this issue can be resolved by create
such ReorderBufferTXNs in SnapBuildProcessRunningXacts.Thank you for the idea and patch!
Thanks you both for your new patches proposal!
I liked Sawada's one but also do "prefer" Horiguchi's one.
It's much simpler than mine. I think that creating an entry of a
catalog-changed transaction in the reorder buffer before
SunapBuildCommitTxn() is the right direction.
+1
After more thought, given DDLs are not likely to happen than DML in
practice, probably we can always mark both the top transaction and its
subtransactions as containing catalog changes if the commit record has
XACT_XINFO_HAS_INVALS? I believe this is not likely to lead to
overhead in practice. That way, the patch could be more simple and
doesn't need the change of AssertTXNLsnOrder().I've attached another PoC patch. Also, I've added the tests for this
issue in test_decoding.
Thanks!
It looks good to me, just have a remark about the comment:
+ /*
+ * Mark the top transaction and its subtransactions as containing
catalog
+ * changes, if the commit record has invalidation message. This is
necessary
+ * for the case where we decode only the commit record of the
transaction
+ * that actually has done catalog changes.
+ */What about?
/*
* Mark the top transaction and its subtransactions as containing
catalog
* changes, if the commit record has invalidation message. This is
necessary
* for the case where we did not decode the transaction that did
the catalog
* change(s) (the decoding restarted after). So that we are
decoding only the
* commit record of the transaction that actually has done catalog
changes.
*/
Thanks
Bertrand
On 10/10/21 23:27, Masahiko Sawada wrote:
After more thought, given DDLs are not likely to happen than DML in
practice, ...
I haven't looked closely at the patch, but I'd be careful about
workloads where people create and drop "temporary tables". I've seen
this pattern used a few times, especially by developers who came from a
SQL server background, for some reason.
I certainly don't think we need to optimize for this workload - which is
not a best practice on PostreSQL. I'd just want to be careful not to
make PostgreSQL logical replication crumble underneath it, if PG was
previously keeping up with difficulty. That would be a sad upgrade
experience!
-Jeremy
On Monday, October 11, 2021 3:28 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Fri, Oct 8, 2021 at 4:50 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:At Thu, 7 Oct 2021 13:20:14 +0900, Masahiko Sawada
<sawada.mshk@gmail.com> wrote inAnother idea to fix this problem would be that before calling
SnapBuildCommitTxn() we create transaction entries in ReorderBuffer
for (sub)transactions whose COMMIT record hasXACT_XINFO_HAS_INVALS,
and then mark all of them as catalog-changed by calling
ReorderBufferXidSetCatalogChanges(). I've attached a PoC patch for
this idea. What the patch does is essentially the same as what the
proposed patch does. But the patch doesn't modify the
SnapBuildCommitTxn(). And we remember the list of last running
transactions in reorder buffer and the list is periodically purged
during decoding RUNNING_XACTS records, eventually making it empty.I came up with the third way. SnapBuildCommitTxn already properly
handles the case where a ReorderBufferTXN with
RBTXN_HAS_CATALOG_CHANGES. So this issue can be resolved bycreate
such ReorderBufferTXNs in SnapBuildProcessRunningXacts.
Thank you for the idea and patch!
It's much simpler than mine. I think that creating an entry of a catalog-changed
transaction in the reorder buffer before
SunapBuildCommitTxn() is the right direction.After more thought, given DDLs are not likely to happen than DML in practice,
probably we can always mark both the top transaction and its subtransactions
as containing catalog changes if the commit record has
XACT_XINFO_HAS_INVALS? I believe this is not likely to lead to overhead in
practice. That way, the patch could be more simple and doesn't need the
change of AssertTXNLsnOrder().I've attached another PoC patch. Also, I've added the tests for this issue in
test_decoding.
I also felt that your patch addresses the problem in a good way.
Even without setting xid by NEW_CID decoding like in the original scenario,
we can set catalog change flag.
One really minor comment I have is,
in DecodeCommit(), you don't need to declar i. It's defined at the top of the function.
+ for (int i = 0; i < parsed->nsubxacts; i++)
Best Regards,
Takamichi Osumi
On Wed, Oct 13, 2021 at 7:55 AM Jeremy Schneider
<schneider@ardentperf.com> wrote:
On 10/10/21 23:27, Masahiko Sawada wrote:
After more thought, given DDLs are not likely to happen than DML in
practice, ...I haven't looked closely at the patch, but I'd be careful about
workloads where people create and drop "temporary tables". I've seen
this pattern used a few times, especially by developers who came from a
SQL server background, for some reason.
True. But since the snapshot builder is designed on the same
assumption it would not be problematic. It keeps track of the
committed catalog modifying transaction instead of keeping track of
all running transactions. See the header comment of snapbuild.c
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
At Mon, 11 Oct 2021 15:27:41 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
.
On Fri, Oct 8, 2021 at 4:50 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:I came up with the third way. SnapBuildCommitTxn already properly
handles the case where a ReorderBufferTXN with
RBTXN_HAS_CATALOG_CHANGES. So this issue can be resolved by create
such ReorderBufferTXNs in SnapBuildProcessRunningXacts.Thank you for the idea and patch!
It's much simpler than mine. I think that creating an entry of a
catalog-changed transaction in the reorder buffer before
SunapBuildCommitTxn() is the right direction.After more thought, given DDLs are not likely to happen than DML in
practice, probably we can always mark both the top transaction and its
subtransactions as containing catalog changes if the commit record has
XACT_XINFO_HAS_INVALS? I believe this is not likely to lead to
overhead in practice. That way, the patch could be more simple and
doesn't need the change of AssertTXNLsnOrder().I've attached another PoC patch. Also, I've added the tests for this
issue in test_decoding.
Thanks for the test script. (I did that with TAP framework but
isolation tester version is simpler.)
It adds a call to ReorderBufferAssignChild but usually subtransactions
are assigned to top level elsewherae. Addition to that
ReorderBufferCommitChild() called just later does the same thing. We
are adding the third call to the same function, which looks a bit odd.
And I'm not sure it is wise to mark all subtransactions as "catalog
changed" always when the top transaction is XACT_XINFO_HAS_INVALS. The
reason I did that in the snapshiot building phase is to prevent adding
to DecodeCommit an extra code that is needed only while any
transaction running since before replication start is surviving.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Thursday, October 14, 2021 11:21 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
At Mon, 11 Oct 2021 15:27:41 +0900, Masahiko Sawada
<sawada.mshk@gmail.com> wrote inOn Fri, Oct 8, 2021 at 4:50 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:I came up with the third way. SnapBuildCommitTxn already properly
handles the case where a ReorderBufferTXN with
RBTXN_HAS_CATALOG_CHANGES. So this issue can be resolved bycreate
such ReorderBufferTXNs in SnapBuildProcessRunningXacts.
Thank you for the idea and patch!
It's much simpler than mine. I think that creating an entry of a
catalog-changed transaction in the reorder buffer before
SunapBuildCommitTxn() is the right direction.After more thought, given DDLs are not likely to happen than DML in
practice, probably we can always mark both the top transaction and its
subtransactions as containing catalog changes if the commit record has
XACT_XINFO_HAS_INVALS? I believe this is not likely to lead to
overhead in practice. That way, the patch could be more simple and
doesn't need the change of AssertTXNLsnOrder().I've attached another PoC patch. Also, I've added the tests for this
issue in test_decoding.Thanks for the test script. (I did that with TAP framework but isolation tester
version is simpler.)It adds a call to ReorderBufferAssignChild but usually subtransactions are
assigned to top level elsewherae. Addition to that
ReorderBufferCommitChild() called just later does the same thing. We are
adding the third call to the same function, which looks a bit odd.
It can be odd. However, we
have a check at the top of ReorderBufferAssignChild
to judge if the sub transaction is already associated or not
and skip the processings if it is.
And I'm not sure it is wise to mark all subtransactions as "catalog changed"
always when the top transaction is XACT_XINFO_HAS_INVALS.
In order to avoid this,
can't we have a new flag (for example, in reorderbuffer struct) to check
if we start decoding from RUNNING_XACTS, which is similar to the first patch of [1]/messages/by-id/81D0D8B0-E7C4-4999-B616-1E5004DBDCD2@amazon.com
and use it at DecodeCommit ? This still leads to some extra specific codes added
to DecodeCommit and this solution becomes a bit similar to other previous patches though.
[1]: /messages/by-id/81D0D8B0-E7C4-4999-B616-1E5004DBDCD2@amazon.com
Best Regards,
Takamichi Osumi
