Horton

Thread

Commits

  1. Clean up more unused variables in perl code

  2. Cleanup perl code from unused variables and routines

  3. Fix comment in 024_add_drop_pub.pl

  1. Cleaning up perl code

    Alexander Law <exclusion@gmail.com> — 2024-05-20T17:00:00Z

    Hello hackers,

Please look at a bunch of unused variables and a couple of other defects
I found in the perl code, maybe you'll find them worth fixing:
contrib/amcheck/t/001_verify_heapam.pl
$result # unused since introduction in 866e24d47
unused sub:
get_toast_for # not used since 860593ec3

contrib/amcheck/t/002_cic.pl
$result # orphaned since 7f580aa5d

src/backend/utils/activity/generate-wait_event_types.pl
$note, $note_name # unused since introduction in fa8892847

src/bin/pg_dump/t/003_pg_dump_with_server.pl
$cmd, $stdout, $stderr, $result # unused since introduction in 2f9eb3132

src/bin/pg_dump/t/005_pg_dump_filterfile.pl
$cmd, $stdout, $stderr, $result # unused since introduciton in a5cf808be

src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl
$slapd, $ldap_schema_dir # unused since introduction in 419a8dd81

src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
$clearpass # orphaned since b846091fd

src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
$ostmt # unused since introduction in 47bb9db75

src/test/recovery/t/021_row_visibility.pl
$ret # unused since introduction in 7b28913bc

src/test/recovery/t/032_relfilenode_reuse.pl
$ret # unused since introduction in e2f65f425

src/test/recovery/t/035_standby_logical_decoding.pl
$stdin, $ret, $slot # unused since introduction in fcd77d532
$subscriber_stdin, $subscriber_stdout, $subscriber_stderr # unused since introduction in 376dc8205

src/test/subscription/t/024_add_drop_pub.pl
invalid reference in a comment:
tab_drop_refresh -> tab_2 # introduced with 1046a69b3
(invalid since v6-0001-...patch in the commit's thread)

src/tools/msvc_gendef.pl
@def # unused since introduction in 933b46644?

I've attached a patch with all of these changes (tested with meson build
on Windows and check-world on Linux).

Best regards,
Alexander

  2. Re: Cleaning up perl code

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2024-05-20T20:39:34Z

    Alexander Lakhin <exclusion@gmail.com> writes:

> Hello hackers,
>
> Please look at a bunch of unused variables and a couple of other defects
> I found in the perl code, maybe you'll find them worth fixing:

Nice cleanup!  Did you use some static analysis tool, or did look for
them manually?  If I add [Variables::ProhibitUnusedVariables] to
src/tools/perlcheck/perlcriticrc, it finds a few more, see the attached
patch.

The scripts parsing errcodes.txt really should be refactored into using
a common module, but that's a patch for another day.

- ilmari

  3. Re: Cleaning up perl code

    Alexander Law <exclusion@gmail.com> — 2024-05-21T03:00:00Z

    Hello Dagfinn,

Thank you for paying attention to it and improving the possible fix!

20.05.2024 23:39, Dagfinn Ilmari Mannsåker wrote:
> Nice cleanup!  Did you use some static analysis tool, or did look for
> them manually?

I reviewed my collection of unica I gathered for several months, but had
found some of them too minor/requiring more analysis.
Then I added more with perlcritic's policy UnusedVariables, and also
checked for unused subs with a script from blogs.perl.org (and it confirmed
my only previous find of that kind).

>   If I add [Variables::ProhibitUnusedVariables] to
> src/tools/perlcheck/perlcriticrc, it finds a few more, see the attached
> patch.

Yes, I saw unused $sqlstates, but decided that they are meaningful enough
to stay. Though maybe enabling ProhibitUnusedVariables justifies fixing
them too.

> The scripts parsing errcodes.txt really should be refactored into using
> a common module, but that's a patch for another day.

Agree, and I would leave 005_negotiate_encryption.pl (with $node_conf,
$server_config unused since d39a49c1e) aside for another day too.

Best regards,
Alexander

  4. Re: Cleaning up perl code

    Michael Paquier <michael@paquier.xyz> — 2024-05-21T05:33:16Z

    On Tue, May 21, 2024 at 06:00:00AM +0300, Alexander Lakhin wrote:
> I reviewed my collection of unica I gathered for several months, but had
> found some of them too minor/requiring more analysis.
> Then I added more with perlcritic's policy UnusedVariables, and also
> checked for unused subs with a script from blogs.perl.org (and it confirmed
> my only previous find of that kind).

Nice catches from both of you.  The two ones in
generate-wait_event_types.pl are caused by me, actually.

Not sure about the changes in the errcodes scripts, though.  The
current state of thing can be also useful when it comes to debugging
the parsing, and it does not hurt to keep the parsing rules the same
across the board.

>> The scripts parsing errcodes.txt really should be refactored into using
>> a common module, but that's a patch for another day.
> 
> Agree, and I would leave 005_negotiate_encryption.pl (with $node_conf,
> $server_config unused since d39a49c1e) aside for another day too.

I'm not sure about these ones as each one of these scripts have their
own local tweaks.  Now, if there is a cleaner picture with a .pm
module I don't see while reading the whole, why not as long as it
improves the code.
--
Michael

  5. Re: Cleaning up perl code

    Michael Paquier <michael@paquier.xyz> — 2024-05-24T05:09:49Z

    On Tue, May 21, 2024 at 02:33:16PM +0900, Michael Paquier wrote:
> Nice catches from both of you.  The two ones in
> generate-wait_event_types.pl are caused by me, actually.
> 
> Not sure about the changes in the errcodes scripts, though.  The
> current state of thing can be also useful when it comes to debugging
> the parsing, and it does not hurt to keep the parsing rules the same
> across the board.

For now, I have staged for commit the attached, that handles most of
the changes from Alexander (msvc could go for more cleanup?).  I'll
look at the changes from Dagfinn after that, including if perlcritic
could be changed.  I'll handle the first part when v18 opens up, as
that's cosmetic.

The incorrect comment in 024_add_drop_pub.pl has been fixed as of
53785d2a2aaa, as that was a separate issue.
--
Michael

  6. Re: Cleaning up perl code

    Michael Paquier <michael@paquier.xyz> — 2024-07-02T01:11:46Z

    On Fri, May 24, 2024 at 02:09:49PM +0900, Michael Paquier wrote:
> For now, I have staged for commit the attached, that handles most of
> the changes from Alexander (msvc could go for more cleanup?).

This one has been applied as of 0c1aca461481 now that v18 is
open.

> I'll look at the changes from Dagfinn after that, including if perlcritic
> could be changed.  I'll handle the first part when v18 opens up, as
> that's cosmetic.

I'm still biased about the second set of changes proposed here,
though.  ProhibitUnusedVariables would have benefits when writing perl
code in terms of clarity because we would avoid useless stuff, but it
seems to me that we should put more efforts into the unification of
the errcodes parsing paths first to have a cleaner long-term picture.

That's not directly the fault of this proposal that we have the same
parsing rules spread across three PL languages, so perhaps what's
proposed is fine as-is, at the end.

Any thoughts or comments from others more familiar with
ProhibitUnusedVariables?
--
Michael

  7. Re: Cleaning up perl code

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2024-07-02T12:55:25Z

    Michael Paquier <michael@paquier.xyz> writes:

> On Fri, May 24, 2024 at 02:09:49PM +0900, Michael Paquier wrote:
>> For now, I have staged for commit the attached, that handles most of
>> the changes from Alexander (msvc could go for more cleanup?).
>
> This one has been applied as of 0c1aca461481 now that v18 is
> open.
>
>> I'll look at the changes from Dagfinn after that, including if perlcritic
>> could be changed.  I'll handle the first part when v18 opens up, as
>> that's cosmetic.

For clarity, I've rebased my addional unused-variable changes (except
the errcodes-related ones, see below) onto current master, and split it
into separate commits with detailed explaiations for each file file, see
attached.

> I'm still biased about the second set of changes proposed here,
> though.  ProhibitUnusedVariables would have benefits when writing perl
> code in terms of clarity because we would avoid useless stuff, but it
> seems to me that we should put more efforts into the unification of
> the errcodes parsing paths first to have a cleaner long-term picture.
>
> That's not directly the fault of this proposal that we have the same
> parsing rules spread across three PL languages, so perhaps what's
> proposed is fine as-is, at the end.

It turns out there are a couple more places that parse errcodes.txt,
namely doc/src/sgml/generate-errcodes-table.pl and
src/backend/utils/generate-errcodes.pl.  I'll have a go refactoring all
of these into a common function à la Catalog::ParseHeader() that returns
a data structure these scripts can use as as appropriate.

> Any thoughts or comments from others more familiar with
> ProhibitUnusedVariables?

Relatedly, I also had a look at prohibiting unused regex captures
(RegularExpressions::ProhibitUnusedCapture), which found a few real
cases, but also lots of false positives in Catalog.pm, because it
doesn't understand that %+ uses all named captures, so I won't propose a
patch for that until that's fixed upstream
(https://github.com/Perl-Critic/Perl-Critic/pull/1065).

- ilmari

  8. Re: Cleaning up perl code

    Andrew Dunstan <andrew@dunslane.net> — 2024-07-02T15:11:22Z

    On 2024-07-02 Tu 8:55 AM, Dagfinn Ilmari Mannsåker wrote:
> Relatedly, I also had a look at prohibiting unused regex captures
> (RegularExpressions::ProhibitUnusedCapture), which found a few real
> cases, but also lots of false positives in Catalog.pm, because it
> doesn't understand that %+ uses all named captures, so I won't propose a
> patch for that until that's fixed upstream
> (https://github.com/Perl-Critic/Perl-Critic/pull/1065).
>

We could mark Catalog.pm with a "## no critic (ProhibitUnusedCapture)" 
and then use the test elsewhere.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

  9. Re: Cleaning up perl code

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2024-07-02T15:38:01Z

    Andrew Dunstan <andrew@dunslane.net> writes:

> On 2024-07-02 Tu 8:55 AM, Dagfinn Ilmari Mannsåker wrote:
>> Relatedly, I also had a look at prohibiting unused regex captures
>> (RegularExpressions::ProhibitUnusedCapture), which found a few real
>> cases, but also lots of false positives in Catalog.pm, because it
>> doesn't understand that %+ uses all named captures, so I won't propose a
>> patch for that until that's fixed upstream
>> (https://github.com/Perl-Critic/Perl-Critic/pull/1065).
>>
>
> We could mark Catalog.pm with a "## no critic (ProhibitUnusedCapture)"
> and then use the test elsewhere.

Yeah, that's what I've done for now. Here's a sequence of patches that
fixes the existing cases of unused captures, and adds the policy and
override.

The seg-validate.pl script seems unused, unmaintained and useless (it
doesn't actually match the syntax accepted by seg, specifcially the (+-)
syntax (which my patch fixes in passing)), so maybe we should just
delete it instead?

> cheers
>
> andrew

 -ilmari

  10. Re: Cleaning up perl code

    Michael Paquier <michael@paquier.xyz> — 2024-07-03T03:44:53Z

    On Tue, Jul 02, 2024 at 01:55:25PM +0100, Dagfinn Ilmari Mannsåker wrote:
> For clarity, I've rebased my addional unused-variable changes (except
> the errcodes-related ones, see below) onto current master, and split it
> into separate commits with detailed explaiations for each file file, see
> attached.

Thanks, I've squashed these three ones into a single commit for now
(good catches for 005_negotiate_encryption.pl, btw), and applied them.
--
Michael

  11. Re: Cleaning up perl code

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2024-07-03T13:20:38Z

    Michael Paquier <michael@paquier.xyz> writes:

> On Tue, Jul 02, 2024 at 01:55:25PM +0100, Dagfinn Ilmari Mannsåker wrote:
>> For clarity, I've rebased my addional unused-variable changes (except
>> the errcodes-related ones, see below) onto current master, and split it
>> into separate commits with detailed explaiations for each file file, see
>> attached.
>
> Thanks, I've squashed these three ones into a single commit for now
> (good catches for 005_negotiate_encryption.pl, btw), and applied them.

Thanks!

- ilmari