Thread

Commits

Simplify some code in pg_rewind when syncing target directory
- 6e52209eb1f8 12.0 landed
Don't force-assign transaction id when exporting a snapshot.
- 6c2003f8a1bb 10.0 cited

Unused parameters & co in code

Michael Paquier <michael@paquier.xyz> — 2019-01-30T07:33:17Z

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

Re: Unused parameters & co in code

Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-30T11:14:05Z

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

Re: Unused parameters & co in code

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-01-30T11:33:47Z

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

Re: Unused parameters & co in code

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-30T14:41:04Z

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

Re: Unused parameters & co in code

Michael Paquier <michael@paquier.xyz> — 2019-01-31T00:30:06Z

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

Re: Unused parameters & co in code

Michael Paquier <michael@paquier.xyz> — 2019-01-31T00:35:50Z

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

Re: Unused parameters & co in code

Michael Paquier <michael@paquier.xyz> — 2019-01-31T06:47:59Z

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

Re: Unused parameters & co in code

Antonin Houska <ah@cybertec.at> — 2019-01-31T09:15:21Z

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

Re: Unused parameters & co in code

Michael Paquier <michael@paquier.xyz> — 2019-02-26T07:31:01Z

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