Cleanup: remove unused fields from nodes
Hi,
While working on the 'reduce nodeToString output' patch, I noticed
that my IDE marked one field in the TidScanState node as 'unused'.
After checking this seemed to be accurate, and I found a few more such
fields in Node structs.
PFA some patches that clean this up: 0001 is plain removal of fields
that are not accessed anywhere anymore, 0002 and up clean up fields
that are effectively write-only, with no effective use inside
PostgreSQL's own code, and no effective usage found on Debian code
search, nor Github code search.
I'm quite confident about the correctness of patches 1 and 3 (no usage
at all, and newly introduced with no meaningful uses), while patches
2, 4, and 5 could be considered 'as designed'.
For those last ones I have no strong opinion for removal or against
keeping them around, this is just to point out we can remove the
fields, as nobody seems to be using them.
/cc Tom Lane and Etsuro Fujita: 2 and 4 were introduced with your
commit afb9249d in 2015.
/cc Amit Kapila: 0003 was introduced with your spearheaded commit
6185c973 this year.
Kind regards,
Matthias van de Meent
0001 removes two old fields that are not in use anywhere anymore, but
at some point these were used.
0002/0004 remove fields in ExecRowMark which were added for FDWs to
use, but there are no FDWs which use this: I could only find two FDWs
who implement RefetchForeignRow, one being BlackHoleFDW, and the other
a no-op implementation in kafka_fdw [0]https://github.com/cohenjo/kafka_fdw/blob/master/src/kafka_fdw.c#L1793. We also don't seem to have
any testing on this feature.
0003 drops the input_finfo field on the new JsonExprState struct. It
wasn't read anywhere, so keeping it around makes little sense IMO.
0005 drops field DeallocateStmt.isall: the value of the field is
implied by !name, and that field was used as such.
[0]: https://github.com/cohenjo/kafka_fdw/blob/master/src/kafka_fdw.c#L1793
Attachments:
v1-0002-Remove-field-ExecRowMark.ermActive.patchapplication/octet-stream; name=v1-0002-Remove-field-ExecRowMark.ermActive.patchDownload
From 4c49150a04765ba471eaadd94ddeda9842f8c677 Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Wed, 20 Mar 2024 15:20:38 +0100
Subject: [PATCH v1 2/5] Remove field ExecRowMark.ermActive
The field didn't have any reader in core code, and no other users showed up in GH
code search either.
---
src/backend/executor/execMain.c | 1 -
src/backend/executor/nodeLockRows.c | 2 --
src/include/nodes/execnodes.h | 1 -
3 files changed, 4 deletions(-)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 4d7c92d63c..8a74d00303 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -908,7 +908,6 @@ InitPlan(QueryDesc *queryDesc, int eflags)
erm->markType = rc->markType;
erm->strength = rc->strength;
erm->waitPolicy = rc->waitPolicy;
- erm->ermActive = false;
ItemPointerSetInvalid(&(erm->curCtid));
erm->ermExtra = NULL;
diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c
index 41754ddfea..7ffedafebb 100644
--- a/src/backend/executor/nodeLockRows.c
+++ b/src/backend/executor/nodeLockRows.c
@@ -106,12 +106,10 @@ lnext:
if (tableoid != erm->relid)
{
/* this child is inactive right now */
- erm->ermActive = false;
ItemPointerSetInvalid(&(erm->curCtid));
continue;
}
}
- erm->ermActive = true;
/* fetch the tuple's ctid */
datum = ExecGetJunkAttribute(slot,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 6365beb701..f8257b60c1 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -754,7 +754,6 @@ typedef struct ExecRowMark
RowMarkType markType; /* see enum in nodes/plannodes.h */
LockClauseStrength strength; /* LockingClause's strength, or LCS_NONE */
LockWaitPolicy waitPolicy; /* NOWAIT and SKIP LOCKED */
- bool ermActive; /* is this mark relevant for current tuple? */
ItemPointerData curCtid; /* ctid of currently locked tuple, if any */
void *ermExtra; /* available for use by relation source node */
} ExecRowMark;
--
2.40.1
v1-0003-Remove-field-JsonExprState.input_finfo.patchapplication/octet-stream; name=v1-0003-Remove-field-JsonExprState.input_finfo.patchDownload
From ef3a81211ff160e2e556cd3a5697cdf4b28e1ad5 Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Mon, 22 Apr 2024 11:41:43 +0200
Subject: [PATCH v1 3/5] Remove field JsonExprState.input_finfo
The field was write-only since it's introduction in 2024 with 6185c973
---
src/backend/executor/execExpr.c | 1 -
src/include/nodes/execnodes.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index eb5ac20824..b9ebc827a7 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -4393,7 +4393,6 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state,
fcinfo->args[2].isnull = false;
fcinfo->context = (Node *) escontext;
- jsestate->input_finfo = finfo;
jsestate->input_fcinfo = fcinfo;
}
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index f8257b60c1..fa1c84bb1a 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1069,7 +1069,6 @@ typedef struct JsonExprState
* RETURNING type input function invocation info when
* JsonExpr.use_io_coercion is true.
*/
- FmgrInfo *input_finfo;
FunctionCallInfo input_fcinfo;
/*
--
2.40.1
v1-0005-Remove-field-DeallocateStmt.isall.patchapplication/octet-stream; name=v1-0005-Remove-field-DeallocateStmt.isall.patchDownload
From 66fb260c0e05fb728395f3b702d202974b7cac1a Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Mon, 22 Apr 2024 16:36:10 +0200
Subject: [PATCH v1 5/5] Remove field DeallocateStmt.isall
The field is only written to, is equivalent to !PointerIsValid(DeallocateStmt.name),
and has no current readers.
---
src/backend/parser/gram.y | 4 ----
src/include/nodes/parsenodes.h | 2 --
2 files changed, 6 deletions(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index e8b619926e..bc84f65a93 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -12179,7 +12179,6 @@ DeallocateStmt: DEALLOCATE name
DeallocateStmt *n = makeNode(DeallocateStmt);
n->name = $2;
- n->isall = false;
n->location = @2;
$$ = (Node *) n;
}
@@ -12188,7 +12187,6 @@ DeallocateStmt: DEALLOCATE name
DeallocateStmt *n = makeNode(DeallocateStmt);
n->name = $3;
- n->isall = false;
n->location = @3;
$$ = (Node *) n;
}
@@ -12197,7 +12195,6 @@ DeallocateStmt: DEALLOCATE name
DeallocateStmt *n = makeNode(DeallocateStmt);
n->name = NULL;
- n->isall = true;
n->location = -1;
$$ = (Node *) n;
}
@@ -12206,7 +12203,6 @@ DeallocateStmt: DEALLOCATE name
DeallocateStmt *n = makeNode(DeallocateStmt);
n->name = NULL;
- n->isall = true;
n->location = -1;
$$ = (Node *) n;
}
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index c5f34efe27..600dce8482 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -4070,8 +4070,6 @@ typedef struct DeallocateStmt
NodeTag type;
/* The name of the plan to remove, NULL if DEALLOCATE ALL */
char *name pg_node_attr(query_jumble_ignore);
- /* true if DEALLOCATE ALL */
- bool isall;
/* token location, or -1 if unknown */
ParseLoc location pg_node_attr(query_jumble_location);
} DeallocateStmt;
--
2.40.1
v1-0001-Remove-some-unused-fields-from-nodes-in-execnodes.patchapplication/octet-stream; name=v1-0001-Remove-some-unused-fields-from-nodes-in-execnodes.patchDownload
From 3781a12cbd461a7fa95b36b952b93b5536dc53f5 Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Wed, 20 Mar 2024 14:37:07 +0100
Subject: [PATCH v1 1/5] Remove some unused fields from nodes in execnodes.h:
TidScanState.tss_htup's last use was removed in 2019 with 2e3da03e
AggState.combinedproj's last use was removed in 2018 with 69c3936a
---
src/include/nodes/execnodes.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index c1a65bad2a..6365beb701 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1846,7 +1846,6 @@ typedef struct TidScanState
int tss_NumTids;
int tss_TidPtr;
ItemPointerData *tss_TidList;
- HeapTupleData tss_htup;
} TidScanState;
/* ----------------
@@ -2530,7 +2529,6 @@ typedef struct AggState
#define FIELDNO_AGGSTATE_ALL_PERGROUPS 53
AggStatePerGroup *all_pergroups; /* array of first ->pergroups, than
* ->hash_pergroup */
- ProjectionInfo *combinedproj; /* projection machinery */
SharedAggInfo *shared_info; /* one entry per worker */
} AggState;
--
2.40.1
v1-0004-Remove-field-ExecRowMark.ermExtra.patchapplication/octet-stream; name=v1-0004-Remove-field-ExecRowMark.ermExtra.patchDownload
From 167d4f9c0b157d77b33563827d20a37e18950b69 Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Mon, 22 Apr 2024 11:53:38 +0200
Subject: [PATCH v1 4/5] Remove field ExecRowMark.ermExtra
The field was write-only since its introduction in 2015 with afb9249d,
and no uses of the field have been found.
---
src/backend/executor/execMain.c | 1 -
src/include/nodes/execnodes.h | 5 +----
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 8a74d00303..9dce7bad51 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -909,7 +909,6 @@ InitPlan(QueryDesc *queryDesc, int eflags)
erm->strength = rc->strength;
erm->waitPolicy = rc->waitPolicy;
ItemPointerSetInvalid(&(erm->curCtid));
- erm->ermExtra = NULL;
Assert(erm->rti > 0 && erm->rti <= estate->es_range_table_size &&
estate->es_rowmarks[erm->rti - 1] == NULL);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index fa1c84bb1a..52d7dca38d 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -736,9 +736,7 @@ typedef struct EState
* PlanRowMark for details about most of the fields. In addition to fields
* directly derived from PlanRowMark, we store an activity flag (to denote
* inactive children of inheritance trees), curCtid, which is used by the
- * WHERE CURRENT OF code, and ermExtra, which is available for use by the plan
- * node that sources the relation (e.g., for a foreign table the FDW can use
- * ermExtra to hold information).
+ * WHERE CURRENT OF code.
*
* EState->es_rowmarks is an array of these structs, indexed by RT index,
* with NULLs for irrelevant RT indexes. es_rowmarks itself is NULL if
@@ -755,7 +753,6 @@ typedef struct ExecRowMark
LockClauseStrength strength; /* LockingClause's strength, or LCS_NONE */
LockWaitPolicy waitPolicy; /* NOWAIT and SKIP LOCKED */
ItemPointerData curCtid; /* ctid of currently locked tuple, if any */
- void *ermExtra; /* available for use by relation source node */
} ExecRowMark;
/*
--
2.40.1
Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
0001 removes two old fields that are not in use anywhere anymore, but
at some point these were used.
+1. They're not being initialized, so they're useless and confusing.
0002/0004 remove fields in ExecRowMark which were added for FDWs to
use, but there are no FDWs which use this: I could only find two FDWs
who implement RefetchForeignRow, one being BlackHoleFDW, and the other
a no-op implementation in kafka_fdw [0]. We also don't seem to have
any testing on this feature.
I'm kind of down on removing either of these. ermExtra is explicitly
intended for extensions to use, and just because we haven't found any
users doesn't mean there aren't any, or might not be next week.
Similarly, ermActive seems like it's at least potentially useful:
is there another way for onlookers to discover that state?
0003 drops the input_finfo field on the new JsonExprState struct. It
wasn't read anywhere, so keeping it around makes little sense IMO.
+1. The adjacent input_fcinfo field has this info if anyone needs it.
0005 drops field DeallocateStmt.isall: the value of the field is
implied by !name, and that field was used as such.
Seems reasonable.
I think it would be a good idea to push 0003 for v17, just so nobody
grows an unnecessary dependency on that field. 0001 and 0005 could
be left for v18, but on the other hand they're so trivial that it
could also be sensible to just push them to get them out of the way.
regards, tom lane
On Mon, 22 Apr 2024 at 17:41, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
0002/0004 remove fields in ExecRowMark which were added for FDWs to
use, but there are no FDWs which use this: I could only find two FDWs
who implement RefetchForeignRow, one being BlackHoleFDW, and the other
a no-op implementation in kafka_fdw [0]. We also don't seem to have
any testing on this feature.I'm kind of down on removing either of these. ermExtra is explicitly
intended for extensions to use, and just because we haven't found any
users doesn't mean there aren't any, or might not be next week.
That's a good point, and also why I wasn't 100% sure removing it was a
good idea. I'm not quite sure why this would be used (rather than the
internal state of the FDW, or no state at all), but haven't looked
very deep into it either, so I'm quite fine with not channging that.
Similarly, ermActive seems like it's at least potentially useful:
is there another way for onlookers to discover that state?
The ermActive field is always true when RefetchForeignRow is called
(in ExecLockRows(), in nodeLockRows.c), and we don't seem to care
about the value of the field afterwards. Additionally, we always set
erm->curCtid to a valid value when ermActive is also first set in that
code path.
In all, it feels like a duplicative field with no real uses inside
PostgreSQL itself. If an extension (FDW) needs it, it should probably
use ermExtra instead, as ermActive seemingly doesn't carry any
meaningful value into the FDW call.
I think it would be a good idea to push 0003 for v17, just so nobody
grows an unnecessary dependency on that field. 0001 and 0005 could
be left for v18, but on the other hand they're so trivial that it
could also be sensible to just push them to get them out of the way.
Beta 1 scheduled to be released for quite some time, so I don't think
there are any problems with fixing these kinds of minor issues in the
provisional ABI for v17.
Kind regards,
Matthias van de Meent
On Mon, Apr 22, 2024 at 06:46:27PM +0200, Matthias van de Meent wrote:
On Mon, 22 Apr 2024 at 17:41, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
0002/0004 remove fields in ExecRowMark which were added for FDWs to
use, but there are no FDWs which use this: I could only find two FDWs
who implement RefetchForeignRow, one being BlackHoleFDW, and the other
a no-op implementation in kafka_fdw [0]. We also don't seem to have
any testing on this feature.I'm kind of down on removing either of these. ermExtra is explicitly
intended for extensions to use, and just because we haven't found any
users doesn't mean there aren't any, or might not be next week.That's a good point, and also why I wasn't 100% sure removing it was a
good idea. I'm not quite sure why this would be used (rather than the
internal state of the FDW, or no state at all), but haven't looked
very deep into it either, so I'm quite fine with not channging that.
Custom nodes are one extra possibility? I'd leave ermActive and
ermExtra be.
I think it would be a good idea to push 0003 for v17, just so nobody
grows an unnecessary dependency on that field. 0001 and 0005 could
be left for v18, but on the other hand they're so trivial that it
could also be sensible to just push them to get them out of the way.Beta 1 scheduled to be released for quite some time, so I don't think
there are any problems with fixing these kinds of minor issues in the
provisional ABI for v17.
Tweaking the APIs should be OK until GA, as long as we agree that the
current interfaces can be improved.
0003 is new in v17, so let's apply it now. I don't see much a strong
argument in waiting for the removal of 0001 and 0005, either, to keep
the interfaces cleaner moving on. However, this is not a regression
and these have been around for years, so I'd suggest for v18 to open
before moving on with the removal.
I was wondering for a bit about how tss_htup could be abused in the
open, and the only references I can see come from forks of the
pre-2019 area, where this uses TidNext(). As a whole, ripping it out
does not stress me much.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Mon, Apr 22, 2024 at 06:46:27PM +0200, Matthias van de Meent wrote:
On Mon, 22 Apr 2024 at 17:41, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think it would be a good idea to push 0003 for v17, just so nobody
grows an unnecessary dependency on that field. 0001 and 0005 could
be left for v18, but on the other hand they're so trivial that it
could also be sensible to just push them to get them out of the way.
Tweaking the APIs should be OK until GA, as long as we agree that the
current interfaces can be improved.
0003 is new in v17, so let's apply it now. I don't see much a strong
argument in waiting for the removal of 0001 and 0005, either, to keep
the interfaces cleaner moving on. However, this is not a regression
and these have been around for years, so I'd suggest for v18 to open
before moving on with the removal.
I went ahead and pushed 0001 and 0003, figuring there was little
point in waiting on 0001. I'd intended to push 0005 (remove "isall")
as well, but it failed check-world:
diff -U3 /home/postgres/pgsql/contrib/pg_stat_statements/expected/utility.out /home/postgres/pgsql/contrib/pg_stat_statements/results/utility.out
--- /home/postgres/pgsql/contrib/pg_stat_statements/expected/utility.out 2023-12-08 15:14:55.689347888 -0500
+++ /home/postgres/pgsql/contrib/pg_stat_statements/results/utility.out 2024-04-23 12:17:22.187721947 -0400
@@ -536,12 +536,11 @@
SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
calls | rows | query
-------+------+----------------------------------------------------
- 2 | 0 | DEALLOCATE $1
- 2 | 0 | DEALLOCATE ALL
+ 4 | 0 | DEALLOCATE $1
2 | 2 | PREPARE stat_select AS SELECT $1 AS a
1 | 1 | SELECT $1 as a
1 | 1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
-(5 rows)
+(4 rows)SELECT pg_stat_statements_reset() IS NOT NULL AS t;
That is, query jumbling no longer distinguishes "DEALLOCATE x" from
"DEALLOCATE ALL", because the DeallocateStmt.name field is marked
query_jumble_ignore. Now maybe that's fine, but it's a point
we'd not considered so far in this thread. Thoughts?
regards, tom lane
On Tue, Apr 23, 2024 at 01:01:04PM -0400, Tom Lane wrote:
That is, query jumbling no longer distinguishes "DEALLOCATE x" from
"DEALLOCATE ALL", because the DeallocateStmt.name field is marked
query_jumble_ignore. Now maybe that's fine, but it's a point
we'd not considered so far in this thread. Thoughts?
And of course, I've managed to forget about bb45156f342c and the
reason behind the addition of the field is to be able to make the
difference between the named and ALL cases for DEALLOCATE, around
here:
/messages/by-id/ZNq9kRwWbKzvR+2a@paquier.xyz
This is new in v17, so perhaps it could be changed, but I think that's
important to make the difference here for monitoring purposes as
DEALLOCATE ALL could be used as a way to clean up prepared statements
in connection poolers (for example, pgbouncer's server_reset_query).
And doing this tweak in the Node structure of DeallocateStmt is
simpler than having to maintain a new pg_node_attr for query jumbling.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Tue, Apr 23, 2024 at 01:01:04PM -0400, Tom Lane wrote:
That is, query jumbling no longer distinguishes "DEALLOCATE x" from
"DEALLOCATE ALL", because the DeallocateStmt.name field is marked
query_jumble_ignore. Now maybe that's fine, but it's a point
we'd not considered so far in this thread. Thoughts?
And of course, I've managed to forget about bb45156f342c and the
reason behind the addition of the field is to be able to make the
difference between the named and ALL cases for DEALLOCATE, around
here:
/messages/by-id/ZNq9kRwWbKzvR+2a@paquier.xyz
Hah. Seems like the comment for isall needs to explain that it
exists for this purpose, so we don't make this mistake again.
regards, tom lane
On Tue, Apr 23, 2024 at 11:03:40PM -0400, Tom Lane wrote:
Hah. Seems like the comment for isall needs to explain that it
exists for this purpose, so we don't make this mistake again.
How about something like the attached?
--
Michael
Attachments:
comment.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index c5f34efe27..512b0ef33f 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -4070,7 +4070,10 @@ typedef struct DeallocateStmt
NodeTag type;
/* The name of the plan to remove, NULL if DEALLOCATE ALL */
char *name pg_node_attr(query_jumble_ignore);
- /* true if DEALLOCATE ALL */
+ /*
+ * True if DEALLOCATE ALL, required to make the difference between ALL
+ * and a named DEALLOCATE in query jumbling.
+ */
bool isall;
/* token location, or -1 if unknown */
ParseLoc location pg_node_attr(query_jumble_location);
On Wed, 24 Apr 2024 at 09:28, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Apr 23, 2024 at 11:03:40PM -0400, Tom Lane wrote:
Hah. Seems like the comment for isall needs to explain that it
exists for this purpose, so we don't make this mistake again.How about something like the attached?
LGTM.
Thanks,
Matthias
Michael Paquier <michael@paquier.xyz> writes:
On Tue, Apr 23, 2024 at 11:03:40PM -0400, Tom Lane wrote:
Hah. Seems like the comment for isall needs to explain that it
exists for this purpose, so we don't make this mistake again.
How about something like the attached?
I was thinking about wording like
* True if DEALLOCATE ALL. This is redundant with "name == NULL",
* but we make it a separate field so that exactly this condition
* (and not the precise name) will be accounted for in query jumbling.
regards, tom lane
On Wed, Apr 24, 2024 at 08:31:57AM -0400, Tom Lane wrote:
I was thinking about wording like
* True if DEALLOCATE ALL. This is redundant with "name == NULL",
* but we make it a separate field so that exactly this condition
* (and not the precise name) will be accounted for in query jumbling.
Fine by me. I've just used that and applied a patch to doing so.
Thanks.
--
Michael