Thread: Unused parameters & co in code
Hi all, I just got a run with CFLAGS with the following options: -Wunused-function -Wunused-variable -Wunused-const-variable -Wno-unused-result -Wunused-parameter -Wunused-macros And while this generates a lot of noise for callbacks and depending on the environment used, this is also pointing to things which could be simplified: Unused arguments in functions: heap_getsysattr() => tupleDesc brin_start_evacuating_page() => idxRel UpdateIndexRelation() => parentIndexOid SerializeReindexState() => maxsize ginRedoInsertEntry() => isLeaf ginReadTuple() => ginstate, attnum startScanKey() => ginstate resolve_unique_index_expr() => heapRel gistGetNodeBuffer() => giststate heap_tuple_needs_freeze() => buf log_heap_visible() => rnode transformSQLValueFunction() => pstate HeapTupleSatisfiesSelf() => snapshot, buffer HeapTupleSatisfiesAny() (Style enforced by HeapTupleSatisfiesVisibility) _hash_get_newbucket_from_oldbucket => rel RelationPutHeapTuple => relation checkNameSpaceConflicts => pstate toast_delete_datum => rel visibilitymap_clear => rel _bt_relbuf => rel (Quite some cleanup here for the btree code) ReindexPartitionedIndex => parentIdx assignOperTypes => typeinfo storeProcedures => amoid dropOperators => amoid dropProcedures => amoid ATExecColumnDefault => lockmode rename_constraint_internal => recursing ATExecDropIdentity => recursing ATPrepSetStatistics => colNum, newValue, lockmode ATExecAlterColumnGenericOptions => lockmode ATPostAlterTypeParse => lockmode ATExecEnableDisableRule => lockmode sendAuthRequest => port (Quite some simplification) get_qual_from_partbound => rel alloc_edge_table, free_edge_table, gimme_edge, remove_gene => root set_tablefunc_pathlist => rte cost_bitmap_and_node cost_gather_merge cost_gather eclass_useful_for_merging => root initial_cost_hashjoin clear_external_function_hash => filehandle (could be extended) pg_SASL_init => payloadlen reached_end_position => timeline, segment_finished syncTargetDirectory => argv0 parseAclItem => name describeAggregates => verbose blackholeNoticeProcessor => arg There are also some unused constants and macros: PREFER in regcomp.c STATHDRSIZE in tsvector_op.c NAMESPACE_SQLXML in xml.c (here for documentation) messageEquals in parallel.c For some of these functions, the format is determined by the context, take HeapTupleSatisfiesSelf or some planner routines. For others, it usually comes from some code deleted which did not actually correct the related function. Anyway, simplifying all that means usually a back-patch pain. However, in some cases, like for example _bt_relbuf, ReindexPartitionedIndex or sendAuthRequest it can simplify the code as there is no need to move some extra arguments around from an upper layer. Fixing all of these makes little sense, still it seems to me that the cases where we can get extra simplifications for other code paths calling the functions makes sense and that we should fix them. Thoughts? -- Michael
Attachment
On 2019-Jan-30, Michael Paquier wrote: > ReindexPartitionedIndex => parentIdx I'd not change this one, as we will surely want to do something useful eventually. Not that it matters, but it'd be useless churn. > blackholeNoticeProcessor => arg This one's signature is enforced by PQsetNoticeProcessor. > _bt_relbuf => rel (Quite some cleanup here for the btree code) If this is a worthwhile cleanup, let's see a patch. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 30/01/2019 08:33, Michael Paquier wrote: > I just got a run with CFLAGS with the following options: > -Wunused-function -Wunused-variable -Wunused-const-variable These are part of -Wall. > -Wno-unused-result What is the purpose of this? > -Wunused-parameter I think it's worth cleaning this up a bit, but it needs to be done by hand, unless you want to add a large number of pg_attribute_unused() decorations. > -Wunused-macros I have looked into that in the past. There are a few that were genuinely left behind accidentally, but most are there for completeness or symmetry and don't look like they should be removed. Also you get junk from flex and perl and the like. Needs to be addressed case by case. I can dig up my old branch and make some proposals. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Michael Paquier <michael@paquier.xyz> writes:
> ... while this generates a lot of noise for callbacks and depending on
> the environment used, this is also pointing to things which could be
> simplified:
I'd definitely take this on a case-by-case basis. In the planner
functions you mentioned, for instance, I'd be pretty hesitant to remove
the "root" parameter even if it happens not to be needed today.
We'd probably just end up putting it back in the future, because almost
everything in the planner needs that. I'd only consider removing it in
cases where there was a solid reason to require the function not to need
it ever (as for instance what I just did to flatten_join_alias_vars).
In cases where we can get simplifications of calling layers, and
it doesn't seem likely that we'd have to undo it in future, then
probably it's worth the trouble to change.
regards, tom lane
On Wed, Jan 30, 2019 at 09:41:04AM -0500, Tom Lane wrote: > I'd definitely take this on a case-by-case basis. In the planner > functions you mentioned, for instance, I'd be pretty hesitant to remove > the "root" parameter even if it happens not to be needed today. > We'd probably just end up putting it back in the future, because almost > everything in the planner needs that. I'd only consider removing it in > cases where there was a solid reason to require the function not to need > it ever (as for instance what I just did to flatten_join_alias_vars). Definitely agreed, this is a case-by-case. For the callbacks and hooks it makes no sense, the planner ones and a couple of layers like shared memory calculation are just here for symmetry, so most of the report is really noise (I deliberately discarded anything related to bison and generated code of course). > In cases where we can get simplifications of calling layers, and > it doesn't seem likely that we'd have to undo it in future, then > probably it's worth the trouble to change. Some of these are in tablecmds.c, and visibly worth the trouble. The ones in the btree, gin and gist code also could do for some cleanup at quick sight. And these are places where we complain a lot about the complexity of the code. -- Michael
Attachment
On Wed, Jan 30, 2019 at 12:33:47PM +0100, Peter Eisentraut wrote: > On 30/01/2019 08:33, Michael Paquier wrote: >> I just got a run with CFLAGS with the following options: >> -Wunused-function -Wunused-variable -Wunused-const-variable > > These are part of -Wall. The ones selected already generate a lot of junk, so increasing the output is not really a good idea. What I wanted to find out are the spots where we could be able to simplify the code for any unused parameter. As you mention, some parameters are here for symmetry in the declaration, which makes sense in some cases, but for some other cases I think that we may be able to reduce logic complexity, and this gives hints about that. >> -Wno-unused-result > > What is the purpose of this? Not really useful actually as we don't mark anything with warn_unused_result. >> -Wunused-macros > > I have looked into that in the past. There are a few that were > genuinely left behind accidentally, but most are there for completeness > or symmetry and don't look like they should be removed. Also you get > junk from flex and perl and the like. Needs to be addressed case by > case. I can dig up my old branch and make some proposals. Thanks. Maybe I missed some of them. Some macros, like the xml one, are here for documentation purposes, so removing such things does not make sense either. -- Michael
Attachment
On Wed, Jan 30, 2019 at 08:14:05AM -0300, Alvaro Herrera wrote: > On 2019-Jan-30, Michael Paquier wrote: >> ReindexPartitionedIndex => parentIdx > > I'd not change this one, as we will surely want to do something useful > eventually. Not that it matters, but it'd be useless churn. A lot of these things assume that the unused arguments may be useful in the future. >> _bt_relbuf => rel (Quite some cleanup here for the btree code) > > If this is a worthwhile cleanup, let's see a patch. This cleanup is disappointing, so it can be discarded. I looked at all the references I have spotted yesterday, and most of them are not really worth the trouble, still there are some which could be cleaned up. Attached is a set of patches which do some cleanup here and there, I don't propose all of them for commit, some of them are attached just for reference: - 0001 cleans up port in SendAuthRequest. This one is disappointing, so I am fine to discard it. - 0002 works on _bt_relbuf, whose callers don't actually benefit from the cleanup as the relation worked on is always used for different reasons, so it can be discarded. - 0003 works on the code of GIN, which simplifies at least the code, so it could be applied. This removes more than changed. - 0004 also cleans some code for clause parsing, with a negative line output. - 0005 is for pg_rewind, which is some stuff I introduced, so I'd like to clean up my mess and the change is recent :) - 0006 is for tablecmds.c, and something I would like to apply because it reduces some confusion with some recursion arguments which are not needed for constraint handling and inheritance. Most of the complains come from lockmode not being used but all the AtPrep and AtExec routines are rather symmetric so I am not bothering about that. -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> wrote: > Hi all, > > I just got a run with CFLAGS with the following options: > -Wunused-function -Wunused-variable -Wunused-const-variable > -Wno-unused-result -Wunused-parameter -Wunused-macros > > And while this generates a lot of noise for callbacks and depending on > the environment used, this is also pointing to things which could be > simplified: This reminds me that I reported some unused arguments too: https://www.postgresql.org/message-id/6702.1473067599@localhost SnapBuildBuildSnapshot() was fixed in commit 6c2003f8a1b, but the other two are still there. -- Antonin Houska Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26, A-2700 Wiener Neustadt Web: https://www.cybertec-postgresql.com
On Thu, Jan 31, 2019 at 03:47:59PM +0900, Michael Paquier wrote: > - 0001 cleans up port in SendAuthRequest. This one is disappointing, > so I am fine to discard it. > - 0002 works on _bt_relbuf, whose callers don't actually benefit from > the cleanup as the relation worked on is always used for different > reasons, so it can be discarded. > - 0003 works on the code of GIN, which simplifies at least the code, > so it could be applied. This removes more than changed. > - 0004 also cleans some code for clause parsing, with a negative line > output. > - 0005 is for pg_rewind, which is some stuff I introduced, so I'd like > to clean up my mess and the change is recent :) > - 0006 is for tablecmds.c, and something I would like to apply because > it reduces some confusion with some recursion arguments which are not > needed for constraint handling and inheritance. Most of the complains > come from lockmode not being used but all the AtPrep and AtExec > routines are rather symmetric so I am not bothering about that. A note for the archives: I have committed 0005 as 6e52209e because it was directly something I worked on. I have dropped the rest as I am not clear if all those arguments can be useful for future use or not. -- Michael