Cleanup - Removed unused function parameter in reorder buffer & parallel vacuum
Hi,
While checking through the code I found that some of the function
parameters in reorderbuffer & vacuumlazy are not used. I felt this
could be removed. I'm not sure if it is kept for future use or not.
Attached patch contains the changes for the same.
Thoughts?
Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
Attachments:
0001-Removal-unused-function-parameter-in-parallel-vacuum.patchtext/x-patch; charset=US-ASCII; name=0001-Removal-unused-function-parameter-in-parallel-vacuum.patchDownload
From c575e1edda90a529ca24a73d6e99ea6f2af77980 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Fri, 3 Jul 2020 13:29:13 +0530
Subject: [PATCH] Removal unused function parameter in parallel vacuum &
reorder buffer.
Removal unused function parameter in parallel vacuum & reorder buffer. The
function parameters are not used, they can be removed.
---
src/backend/access/heap/vacuumlazy.c | 8 ++---
src/backend/replication/logical/decode.c | 3 +-
src/backend/replication/logical/reorderbuffer.c | 42 ++++++++++++-------------
src/include/replication/reorderbuffer.h | 6 ++--
4 files changed, 29 insertions(+), 30 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 68effca..1bbc459 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -390,7 +390,7 @@ static void update_index_statistics(Relation *Irel, IndexBulkDeleteResult **stat
static LVParallelState *begin_parallel_vacuum(Oid relid, Relation *Irel,
LVRelStats *vacrelstats, BlockNumber nblocks,
int nindexes, int nrequested);
-static void end_parallel_vacuum(Relation *Irel, IndexBulkDeleteResult **stats,
+static void end_parallel_vacuum(IndexBulkDeleteResult **stats,
LVParallelState *lps, int nindexes);
static LVSharedIndStats *get_indstats(LVShared *lvshared, int n);
static bool skip_parallel_vacuum_index(Relation indrel, LVShared *lvshared);
@@ -1712,7 +1712,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
* during parallel mode.
*/
if (ParallelVacuumIsActive(lps))
- end_parallel_vacuum(Irel, indstats, lps, nindexes);
+ end_parallel_vacuum(indstats, lps, nindexes);
/* Update index statistics */
update_index_statistics(Irel, indstats, nindexes);
@@ -3361,8 +3361,8 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
* context, but that won't be safe (see ExitParallelMode).
*/
static void
-end_parallel_vacuum(Relation *Irel, IndexBulkDeleteResult **stats,
- LVParallelState *lps, int nindexes)
+end_parallel_vacuum(IndexBulkDeleteResult **stats, LVParallelState *lps,
+ int nindexes)
{
int i;
diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c
index c2e5e3a..19e1354 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -337,8 +337,7 @@ DecodeStandbyOp(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
(xl_invalidations *) XLogRecGetData(r);
if (!ctx->fast_forward)
- ReorderBufferImmediateInvalidation(ctx->reorder,
- invalidations->nmsgs,
+ ReorderBufferImmediateInvalidation(invalidations->nmsgs,
invalidations->msgs);
}
break;
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 642a1c7..0bc2f15 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -220,7 +220,7 @@ static void ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn,
static ReorderBufferChange *ReorderBufferIterTXNNext(ReorderBuffer *rb, ReorderBufferIterTXNState *state);
static void ReorderBufferIterTXNFinish(ReorderBuffer *rb,
ReorderBufferIterTXNState *state);
-static void ReorderBufferExecuteInvalidations(ReorderBuffer *rb, ReorderBufferTXN *txn);
+static void ReorderBufferExecuteInvalidations(ReorderBufferTXN *txn);
/*
* ---------------------------------------
@@ -235,12 +235,12 @@ static Size ReorderBufferRestoreChanges(ReorderBuffer *rb, ReorderBufferTXN *txn
TXNEntryFile *file, XLogSegNo *segno);
static void ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
char *change);
-static void ReorderBufferRestoreCleanup(ReorderBuffer *rb, ReorderBufferTXN *txn);
+static void ReorderBufferRestoreCleanup(ReorderBufferTXN *txn);
static void ReorderBufferCleanupSerializedTXNs(const char *slotname);
static void ReorderBufferSerializedPath(char *path, ReplicationSlot *slot,
TransactionId xid, XLogSegNo segno);
-static void ReorderBufferFreeSnap(ReorderBuffer *rb, Snapshot snap);
+static void ReorderBufferFreeSnap(Snapshot snap);
static Snapshot ReorderBufferCopySnap(ReorderBuffer *rb, Snapshot orig_snap,
ReorderBufferTXN *txn, CommandId cid);
@@ -437,13 +437,13 @@ ReorderBufferReturnChange(ReorderBuffer *rb, ReorderBufferChange *change)
case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT:
if (change->data.tp.newtuple)
{
- ReorderBufferReturnTupleBuf(rb, change->data.tp.newtuple);
+ ReorderBufferReturnTupleBuf(change->data.tp.newtuple);
change->data.tp.newtuple = NULL;
}
if (change->data.tp.oldtuple)
{
- ReorderBufferReturnTupleBuf(rb, change->data.tp.oldtuple);
+ ReorderBufferReturnTupleBuf(change->data.tp.oldtuple);
change->data.tp.oldtuple = NULL;
}
break;
@@ -458,7 +458,7 @@ ReorderBufferReturnChange(ReorderBuffer *rb, ReorderBufferChange *change)
case REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT:
if (change->data.snapshot)
{
- ReorderBufferFreeSnap(rb, change->data.snapshot);
+ ReorderBufferFreeSnap(change->data.snapshot);
change->data.snapshot = NULL;
}
break;
@@ -466,7 +466,7 @@ ReorderBufferReturnChange(ReorderBuffer *rb, ReorderBufferChange *change)
case REORDER_BUFFER_CHANGE_TRUNCATE:
if (change->data.truncate.relids != NULL)
{
- ReorderBufferReturnRelids(rb, change->data.truncate.relids);
+ ReorderBufferReturnRelids(change->data.truncate.relids);
change->data.truncate.relids = NULL;
}
break;
@@ -505,7 +505,7 @@ ReorderBufferGetTupleBuf(ReorderBuffer *rb, Size tuple_len)
* Free an ReorderBufferTupleBuf.
*/
void
-ReorderBufferReturnTupleBuf(ReorderBuffer *rb, ReorderBufferTupleBuf *tuple)
+ReorderBufferReturnTupleBuf(ReorderBufferTupleBuf *tuple)
{
pfree(tuple);
}
@@ -536,7 +536,7 @@ ReorderBufferGetRelids(ReorderBuffer *rb, int nrelids)
* Free an array of relids.
*/
void
-ReorderBufferReturnRelids(ReorderBuffer *rb, Oid *relids)
+ReorderBufferReturnRelids(Oid *relids)
{
pfree(relids);
}
@@ -1329,7 +1329,7 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* remove entries spilled to disk */
if (rbtxn_is_serialized(txn))
- ReorderBufferRestoreCleanup(rb, txn);
+ ReorderBufferRestoreCleanup(txn);
/* deallocate */
ReorderBufferReturnTXN(rb, txn);
@@ -1477,7 +1477,7 @@ ReorderBufferCopySnap(ReorderBuffer *rb, Snapshot orig_snap,
* Free a previously ReorderBufferCopySnap'ed snapshot
*/
static void
-ReorderBufferFreeSnap(ReorderBuffer *rb, Snapshot snap)
+ReorderBufferFreeSnap(Snapshot snap)
{
if (snap->copied)
pfree(snap);
@@ -1769,7 +1769,7 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId xid,
if (snapshot_now->copied)
{
- ReorderBufferFreeSnap(rb, snapshot_now);
+ ReorderBufferFreeSnap(snapshot_now);
snapshot_now =
ReorderBufferCopySnap(rb, change->data.snapshot,
txn, command_id);
@@ -1820,7 +1820,7 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId xid,
* see new catalog contents, so execute all
* invalidations.
*/
- ReorderBufferExecuteInvalidations(rb, txn);
+ ReorderBufferExecuteInvalidations(txn);
}
break;
@@ -1866,13 +1866,13 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId xid,
AbortCurrentTransaction();
/* make sure there's no cache pollution */
- ReorderBufferExecuteInvalidations(rb, txn);
+ ReorderBufferExecuteInvalidations(txn);
if (using_subtxn)
RollbackAndReleaseCurrentSubTransaction();
if (snapshot_now->copied)
- ReorderBufferFreeSnap(rb, snapshot_now);
+ ReorderBufferFreeSnap(snapshot_now);
/* remove potential on-disk data, and deallocate */
ReorderBufferCleanupTXN(rb, txn);
@@ -1892,13 +1892,13 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId xid,
AbortCurrentTransaction();
/* make sure there's no cache pollution */
- ReorderBufferExecuteInvalidations(rb, txn);
+ ReorderBufferExecuteInvalidations(txn);
if (using_subtxn)
RollbackAndReleaseCurrentSubTransaction();
if (snapshot_now->copied)
- ReorderBufferFreeSnap(rb, snapshot_now);
+ ReorderBufferFreeSnap(snapshot_now);
/* remove potential on-disk data, and deallocate */
ReorderBufferCleanupTXN(rb, txn);
@@ -2010,7 +2010,7 @@ ReorderBufferForget(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn)
* catalog and we need to update the caches according to that.
*/
if (txn->base_snapshot != NULL && txn->ninvalidations > 0)
- ReorderBufferImmediateInvalidation(rb, txn->ninvalidations,
+ ReorderBufferImmediateInvalidation(txn->ninvalidations,
txn->invalidations);
else
Assert(txn->ninvalidations == 0);
@@ -2026,7 +2026,7 @@ ReorderBufferForget(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn)
* transactions (via ReorderBufferForget()).
*/
void
-ReorderBufferImmediateInvalidation(ReorderBuffer *rb, uint32 ninvalidations,
+ReorderBufferImmediateInvalidation(uint32 ninvalidations,
SharedInvalidationMessage *invalidations)
{
bool use_subtxn = IsTransactionOrTransactionBlock();
@@ -2234,7 +2234,7 @@ ReorderBufferAddInvalidations(ReorderBuffer *rb, TransactionId xid,
* in the changestream but we don't know which those are.
*/
static void
-ReorderBufferExecuteInvalidations(ReorderBuffer *rb, ReorderBufferTXN *txn)
+ReorderBufferExecuteInvalidations(ReorderBufferTXN *txn)
{
int i;
@@ -3067,7 +3067,7 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
* Remove all on-disk stored for the passed in transaction.
*/
static void
-ReorderBufferRestoreCleanup(ReorderBuffer *rb, ReorderBufferTXN *txn)
+ReorderBufferRestoreCleanup(ReorderBufferTXN *txn)
{
XLogSegNo first;
XLogSegNo cur;
diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h
index 626ecf4..a288706 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -431,12 +431,12 @@ ReorderBuffer *ReorderBufferAllocate(void);
void ReorderBufferFree(ReorderBuffer *);
ReorderBufferTupleBuf *ReorderBufferGetTupleBuf(ReorderBuffer *, Size tuple_len);
-void ReorderBufferReturnTupleBuf(ReorderBuffer *, ReorderBufferTupleBuf *tuple);
+void ReorderBufferReturnTupleBuf(ReorderBufferTupleBuf *tuple);
ReorderBufferChange *ReorderBufferGetChange(ReorderBuffer *);
void ReorderBufferReturnChange(ReorderBuffer *, ReorderBufferChange *);
Oid *ReorderBufferGetRelids(ReorderBuffer *, int nrelids);
-void ReorderBufferReturnRelids(ReorderBuffer *, Oid *relids);
+void ReorderBufferReturnRelids(Oid *relids);
void ReorderBufferQueueChange(ReorderBuffer *, TransactionId, XLogRecPtr lsn, ReorderBufferChange *);
void ReorderBufferQueueMessage(ReorderBuffer *, TransactionId, Snapshot snapshot, XLogRecPtr lsn,
@@ -461,7 +461,7 @@ void ReorderBufferAddNewTupleCids(ReorderBuffer *, TransactionId, XLogRecPtr ls
CommandId cmin, CommandId cmax, CommandId combocid);
void ReorderBufferAddInvalidations(ReorderBuffer *, TransactionId, XLogRecPtr lsn,
Size nmsgs, SharedInvalidationMessage *msgs);
-void ReorderBufferImmediateInvalidation(ReorderBuffer *, uint32 ninvalidations,
+void ReorderBufferImmediateInvalidation(uint32 ninvalidations,
SharedInvalidationMessage *invalidations);
void ReorderBufferProcessXid(ReorderBuffer *, TransactionId xid, XLogRecPtr lsn);
void ReorderBufferXidSetCatalogChanges(ReorderBuffer *, TransactionId xid, XLogRecPtr lsn);
--
1.8.3.1
On Fri, 3 Jul 2020 at 09:07, vignesh C <vignesh21@gmail.com> wrote:
While checking through the code I found that some of the function
parameters in reorderbuffer & vacuumlazy are not used. I felt this
could be removed. I'm not sure if it is kept for future use or not.
Attached patch contains the changes for the same.
Thoughts?
Unused? To confirm that, everybody that has a logical decoding plugin needs
to check their code so we are certain this is sensible.
Seems like a change with low utility.
--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
Mission Critical Databases
On Fri, 3 Jul 2020 at 17:07, vignesh C <vignesh21@gmail.com> wrote:
Hi,
While checking through the code I found that some of the function
parameters in reorderbuffer & vacuumlazy are not used. I felt this
could be removed. I'm not sure if it is kept for future use or not.
Attached patch contains the changes for the same.
Thoughts?
For the part of parallel vacuum change, it looks good to me.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jul 3, 2020 at 2:06 PM Simon Riggs <simon@2ndquadrant.com> wrote:
On Fri, 3 Jul 2020 at 09:07, vignesh C <vignesh21@gmail.com> wrote:
While checking through the code I found that some of the function
parameters in reorderbuffer & vacuumlazy are not used. I felt this
could be removed. I'm not sure if it is kept for future use or not.
Attached patch contains the changes for the same.
Thoughts?Unused? To confirm that, everybody that has a logical decoding plugin needs to check their code so we are certain this is sensible.
The changes proposed by Vignesh are in ReorderBuffer APIs and some of
them are static functions, so not sure if decoding plugin comes into
the picture.
Seems like a change with low utility.
Yeah, all or most of the ReorderBuffer APIs seem to take the
"ReorderBuffer *" parameter, so not sure if removing from some of them
is useful or not. At least in the current form, they look consistent.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Amit Kapila <amit.kapila16@gmail.com> writes:
On Fri, Jul 3, 2020 at 2:06 PM Simon Riggs <simon@2ndquadrant.com> wrote:
Seems like a change with low utility.
Yeah, all or most of the ReorderBuffer APIs seem to take the
"ReorderBuffer *" parameter, so not sure if removing from some of them
is useful or not. At least in the current form, they look consistent.
Yeah, I agree with that. This makes things less consistent and it seems
like it's not buying much. Are any of these code paths sufficiently hot
that saving a couple of instructions would matter?
In the long run, it seems like the fact that any of these functions
are not using these parameters is an implementation artifact that
could change at any time. So we might just be putting them back
someday, with nothing except code churn and back-patch hazards to
show for our trouble. Or, if you want to argue that a "ReorderBufferXXX"
function is inherently never going to use the ReorderBuffer, why is it
in that module with that name to begin with?
regards, tom lane
On Fri, Jul 3, 2020 at 5:18 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
On Fri, 3 Jul 2020 at 17:07, vignesh C <vignesh21@gmail.com> wrote:
Hi,
While checking through the code I found that some of the function
parameters in reorderbuffer & vacuumlazy are not used. I felt this
could be removed. I'm not sure if it is kept for future use or not.
Attached patch contains the changes for the same.
Thoughts?For the part of parallel vacuum change, it looks good to me.
Unlike ReorderBuffer, this change looks fine to me as well. This is a
quite recent (PG13) change and it would be good to remove it now. So,
I will push this part of change unless I hear any objection in a day
or so.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Sat, Jul 4, 2020 at 12:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Fri, Jul 3, 2020 at 5:18 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:On Fri, 3 Jul 2020 at 17:07, vignesh C <vignesh21@gmail.com> wrote:
Hi,
While checking through the code I found that some of the function
parameters in reorderbuffer & vacuumlazy are not used. I felt this
could be removed. I'm not sure if it is kept for future use or not.
Attached patch contains the changes for the same.
Thoughts?For the part of parallel vacuum change, it looks good to me.
Unlike ReorderBuffer, this change looks fine to me as well. This is a
quite recent (PG13) change and it would be good to remove it now. So,
I will push this part of change unless I hear any objection in a day
or so.
Thanks all for your comments, attached patch has the changes that
excludes the changes made in reorderbuffer.
Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
Attachments:
0001-Removed-unused-function-parameter-in-end_parallel_va.patchtext/x-patch; charset=US-ASCII; name=0001-Removed-unused-function-parameter-in-end_parallel_va.patchDownload
From 70a626eb7a384c2357134142611d511635e5c369 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Sun, 5 Jul 2020 06:11:02 +0530
Subject: [PATCH] Removed unused function parameter in end_parallel_vacuum.
Removed unused function parameter in end_parallel_vacuum. This
function parameter is not used, it can be removed safely.
---
src/backend/access/heap/vacuumlazy.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 68effca..1bbc459 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -390,7 +390,7 @@ static void update_index_statistics(Relation *Irel, IndexBulkDeleteResult **stat
static LVParallelState *begin_parallel_vacuum(Oid relid, Relation *Irel,
LVRelStats *vacrelstats, BlockNumber nblocks,
int nindexes, int nrequested);
-static void end_parallel_vacuum(Relation *Irel, IndexBulkDeleteResult **stats,
+static void end_parallel_vacuum(IndexBulkDeleteResult **stats,
LVParallelState *lps, int nindexes);
static LVSharedIndStats *get_indstats(LVShared *lvshared, int n);
static bool skip_parallel_vacuum_index(Relation indrel, LVShared *lvshared);
@@ -1712,7 +1712,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
* during parallel mode.
*/
if (ParallelVacuumIsActive(lps))
- end_parallel_vacuum(Irel, indstats, lps, nindexes);
+ end_parallel_vacuum(indstats, lps, nindexes);
/* Update index statistics */
update_index_statistics(Irel, indstats, nindexes);
@@ -3361,8 +3361,8 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, LVRelStats *vacrelstats,
* context, but that won't be safe (see ExitParallelMode).
*/
static void
-end_parallel_vacuum(Relation *Irel, IndexBulkDeleteResult **stats,
- LVParallelState *lps, int nindexes)
+end_parallel_vacuum(IndexBulkDeleteResult **stats, LVParallelState *lps,
+ int nindexes)
{
int i;
--
1.8.3.1