On 11/04/2024 16:37, Ranier Vilela wrote:
Em qui., 11 de abr. de 2024 às 09:54, Heikki Linnakangas
<hlinn...@iki.fi <mailto:hlinn...@iki.fi>> escreveu:
On 11/04/2024 15:03, Ranier Vilela wrote:
> Em qua., 10 de abr. de 2024 às 18:28, Heikki Linnakangas
> <hlinn...@iki.fi <mailto:hlinn...@iki.fi> <mailto:hlinn...@iki.fi
<mailto:hlinn...@iki.fi>>> escreveu:
>
> On 10/04/2024 21:07, Ranier Vilela wrote:
> > Hi,
> >
> > Per Coverity.
> >
> > The function ReorderBufferTXNByXid,
> > can return NULL when the parameter *create* is false.
> >
> > In the functions ReorderBufferSetBaseSnapshot
> > and ReorderBufferXidHasBaseSnapshot,
> > the second call to ReorderBufferTXNByXid,
> > pass false to *create* argument.
> >
> > In the function ReorderBufferSetBaseSnapshot,
> > fixed passing true as argument to always return
> > a valid ReorderBufferTXN pointer.
> >
> > In the function ReorderBufferXidHasBaseSnapshot,
> > fixed by checking if the pointer is NULL.
>
> If it's a "known subxid", the top-level XID should already
have its
> ReorderBufferTXN entry, so ReorderBufferTXN() should never
return NULL.
>
> There are several conditions to not return NULL,
> I think trusting never is insecure.
Well, you could make it an elog(ERROR, ..) instead. But the point is
that it should not happen, and if it does for some reason, that's very
suprpising and there is a bug somewhere. In that case, we should *not*
just blindly create it and proceed as if everything was OK.
Thanks for the clarification.
I will then suggest improving robustness,
but avoiding hiding any possible errors that may occur.
I don't much like adding extra runtime checks for "can't happen"
scenarios either. Assertions would be more clear, but in this case the
code would just segfault trying to dereference the NULL pointer, which
is fine for a "can't happen" scenario.
Looking closer, when we identify an XID as a subtransaction, we:
- assign toplevel_xid,
- set RBTXN_IS_SUBXACT, and
- assign toptxn pointer.
ISTM the 'toplevel_xid' and RBTXN_IS_SUBXACT are redundant.
'toplevel_xid' is only used in those two calls that do
ReorderBufferTXNByXid(rb, txn->toplevel_xid,...), and you could replace
those by following the 'toptxn' pointer directly. And RBTXN_IS_SUBXACT
is redundant with (toptxn != NULL). So here's a patch to remove
'toplevel_xid' and RBTXN_IS_SUBXACT altogether.
Amit, you added 'toptxn' in commit c55040ccd017; does this look right to
you?
--
Heikki Linnakangas
Neon (https://neon.tech)
From e8452054a79034d070407775dfcd8b9754602cb9 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Sat, 13 Apr 2024 10:02:41 +0300
Subject: [PATCH 1/1] Remove redundant field and flag from ReorderBufferTXN
RBTXN_IS_SUBXACT and toplevel_xid are redundant with the
ReorderBufferTXN->toptxn field.
---
.../replication/logical/reorderbuffer.c | 28 ++++++++-----------
src/include/replication/reorderbuffer.h | 15 ++--------
2 files changed, 14 insertions(+), 29 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 00a8327e771..fb0dbec155c 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -947,7 +947,7 @@ AssertTXNLsnOrder(ReorderBuffer *rb)
Assert(prev_first_lsn < cur_txn->first_lsn);
/* known-as-subtxn txns must not be listed */
- Assert(!rbtxn_is_known_subxact(cur_txn));
+ Assert(!rbtxn_is_subtxn(cur_txn));
prev_first_lsn = cur_txn->first_lsn;
}
@@ -967,7 +967,7 @@ AssertTXNLsnOrder(ReorderBuffer *rb)
Assert(prev_base_snap_lsn < cur_txn->base_snapshot_lsn);
/* known-as-subtxn txns must not be listed */
- Assert(!rbtxn_is_known_subxact(cur_txn));
+ Assert(!rbtxn_is_subtxn(cur_txn));
prev_base_snap_lsn = cur_txn->base_snapshot_lsn;
}
@@ -1022,7 +1022,7 @@ ReorderBufferGetOldestTXN(ReorderBuffer *rb)
txn = dlist_head_element(ReorderBufferTXN, node, &rb->toplevel_by_lsn);
- Assert(!rbtxn_is_known_subxact(txn));
+ Assert(!rbtxn_is_subtxn(txn));
Assert(txn->first_lsn != InvalidXLogRecPtr);
return txn;
}
@@ -1079,7 +1079,7 @@ ReorderBufferAssignChild(ReorderBuffer *rb, TransactionId xid,
if (!new_sub)
{
- if (rbtxn_is_known_subxact(subtxn))
+ if (rbtxn_is_subtxn(subtxn))
{
/* already associated, nothing to do */
return;
@@ -1095,8 +1095,6 @@ ReorderBufferAssignChild(ReorderBuffer *rb, TransactionId xid,
}
}
- subtxn->txn_flags |= RBTXN_IS_SUBXACT;
- subtxn->toplevel_xid = xid;
Assert(subtxn->nsubtxns == 0);
/* set the reference to top-level transaction */
@@ -1135,8 +1133,6 @@ static void
ReorderBufferTransferSnapToParent(ReorderBufferTXN *txn,
ReorderBufferTXN *subtxn)
{
- Assert(subtxn->toplevel_xid == txn->xid);
-
if (subtxn->base_snapshot != NULL)
{
if (txn->base_snapshot == NULL ||
@@ -1519,7 +1515,7 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
* they originally were happening inside another subtxn, so we won't
* ever recurse more than one level deep here.
*/
- Assert(rbtxn_is_known_subxact(subtxn));
+ Assert(rbtxn_is_subtxn(subtxn));
Assert(subtxn->nsubtxns == 0);
ReorderBufferCleanupTXN(rb, subtxn);
@@ -1629,7 +1625,7 @@ ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prep
* they originally were happening inside another subtxn, so we won't
* ever recurse more than one level deep here.
*/
- Assert(rbtxn_is_known_subxact(subtxn));
+ Assert(rbtxn_is_subtxn(subtxn));
Assert(subtxn->nsubtxns == 0);
ReorderBufferTruncateTXN(rb, subtxn, txn_prepared);
@@ -3143,9 +3139,8 @@ ReorderBufferSetBaseSnapshot(ReorderBuffer *rb, TransactionId xid,
* operate on its top-level transaction instead.
*/
txn = ReorderBufferTXNByXid(rb, xid, true, &is_new, lsn, true);
- if (rbtxn_is_known_subxact(txn))
- txn = ReorderBufferTXNByXid(rb, txn->toplevel_xid, false,
- NULL, InvalidXLogRecPtr, false);
+ if (rbtxn_is_subtxn(txn))
+ txn = rbtxn_get_toptxn(txn);
Assert(txn->base_snapshot == NULL);
txn->base_snapshot = snap;
@@ -3470,9 +3465,8 @@ ReorderBufferXidHasBaseSnapshot(ReorderBuffer *rb, TransactionId xid)
return false;
/* a known subtxn? operate on top-level txn instead */
- if (rbtxn_is_known_subxact(txn))
- txn = ReorderBufferTXNByXid(rb, txn->toplevel_xid, false,
- NULL, InvalidXLogRecPtr, false);
+ if (rbtxn_is_subtxn(txn))
+ txn = rbtxn_get_toptxn(txn);
return txn->base_snapshot != NULL;
}
@@ -3575,7 +3569,7 @@ ReorderBufferLargestStreamableTopTXN(ReorderBuffer *rb)
txn = dlist_container(ReorderBufferTXN, base_snapshot_node, iter.cur);
/* must not be a subtxn */
- Assert(!rbtxn_is_known_subxact(txn));
+ Assert(!rbtxn_is_subtxn(txn));
/* base_snapshot must be set */
Assert(txn->base_snapshot != NULL);
diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h
index 851a001c8bb..a3420ea7b0a 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -160,7 +160,7 @@ typedef struct ReorderBufferChange
/* ReorderBufferTXN txn_flags */
#define RBTXN_HAS_CATALOG_CHANGES 0x0001
-#define RBTXN_IS_SUBXACT 0x0002
+/* 0x0002 is unused */
#define RBTXN_IS_SERIALIZED 0x0004
#define RBTXN_IS_SERIALIZED_CLEAR 0x0008
#define RBTXN_IS_STREAMED 0x0010
@@ -175,12 +175,6 @@ typedef struct ReorderBufferChange
((txn)->txn_flags & RBTXN_HAS_CATALOG_CHANGES) != 0 \
)
-/* Is the transaction known as a subxact? */
-#define rbtxn_is_known_subxact(txn) \
-( \
- ((txn)->txn_flags & RBTXN_IS_SUBXACT) != 0 \
-)
-
/* Has this transaction been spilled to disk? */
#define rbtxn_is_serialized(txn) \
( \
@@ -257,8 +251,8 @@ typedef struct ReorderBufferTXN
/* The transaction's transaction id, can be a toplevel or sub xid. */
TransactionId xid;
- /* Xid of top-level transaction, if known */
- TransactionId toplevel_xid;
+ /* Toplevel transaction for this subxact (NULL for top-level). */
+ struct ReorderBufferTXN *toptxn;
/*
* Global transaction id required for identification of prepared
@@ -295,9 +289,6 @@ typedef struct ReorderBufferTXN
*/
XLogRecPtr end_lsn;
- /* Toplevel transaction for this subxact (NULL for top-level). */
- struct ReorderBufferTXN *toptxn;
-
/*
* LSN of the last lsn at which snapshot information reside, so we can
* restart decoding from there and fully recover this transaction from
--
2.39.2
