cleaning perl code

On Thu, Apr 9, 2020 at 11:44 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

We currently only run perlcritic at severity level 5, which is fairly
permissive. I'd like to reduce that, ideally to, say, level 3, which is
what I use for the buildfarm code.

But let's start by going to severity level 4.

I continue to be skeptical of perlcritic. I think it complains about a
lot of things which don't matter very much. We should consider whether
the effort it takes to keep it warning-clean has proportionate
benefits.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2020-04-09 19:47, Robert Haas wrote:

On Thu, Apr 9, 2020 at 11:44 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

We currently only run perlcritic at severity level 5, which is fairly
permissive. I'd like to reduce that, ideally to, say, level 3, which is
what I use for the buildfarm code.

But let's start by going to severity level 4.

I continue to be skeptical of perlcritic. I think it complains about a
lot of things which don't matter very much. We should consider whether
the effort it takes to keep it warning-clean has proportionate
benefits.

Let's see what the patches look like. At least some of the warnings
look reasonable, especially in the sense that they are things casual
Perl programmers might accidentally do wrong.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 4/9/20 2:26 PM, Peter Eisentraut wrote:

On 2020-04-09 19:47, Robert Haas wrote:

On Thu, Apr 9, 2020 at 11:44 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

We currently only run perlcritic at severity level 5, which is fairly
permissive. I'd like to reduce that, ideally to, say, level 3, which is
what I use for the buildfarm code.

But let's start by going to severity level 4.

I continue to be skeptical of perlcritic. I think it complains about a
lot of things which don't matter very much. We should consider whether
the effort it takes to keep it warning-clean has proportionate
benefits.

Let's see what the patches look like.  At least some of the warnings
look reasonable, especially in the sense that they are things casual
Perl programmers might accidentally do wrong.

OK, I'll prep one or two. I used to be of Robert's opinion, but I've
come around some on it.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Apr 09, 2020 at 11:44:11AM -0400, Andrew Dunstan wrote:

���� 39 Always unpack @_ first

Requiring a "my @args = @_" does not improve this code:

sub CreateSolution
{
...
if ($visualStudioVersion eq '12.00')
{
return new VS2013Solution(@_);
}

���� 30 Code before warnings are enabled

Sounds good. We already require "use strict" before code. Requiring "use
warnings" in the exact same place does not impose much burden.

���� 12 Subroutine "new" called using indirect syntax

No, thanks. "new VS2013Solution(@_)" and "VS2013Solution->new(@_)" are both
fine; enforcing the latter is an ongoing waste of effort.

����� 9 Multiple "package" declarations

This is good advice if you're writing for CPAN, but it would make PostgreSQL
code worse by having us split affiliated code across multiple files.

����� 9 Expression form of "grep"

No, thanks. I'd be happier with the opposite, requiring grep(/x/, $arg)
instead of grep { /x/ } $arg. Neither is worth enforcing.

����� 7 Symbols are exported by default

This is good advice if you're writing for CPAN. For us, it just adds typing.

����� 5 Warnings disabled
����� 4 Magic variable "$/" should be assigned as "local"
����� 4 Comma used to separate statements
����� 2 Readline inside "for" loop
����� 2 Pragma "constant" used
����� 2 Mixed high and low-precedence booleans
����� 2 Don't turn off strict for large blocks of code
����� 1 Magic variable "@a" should be assigned as "local"
����� 1 Magic variable "$|" should be assigned as "local"
����� 1 Magic variable "$\" should be assigned as "local"
����� 1 Magic variable "$?" should be assigned as "local"
����� 1 Magic variable "$," should be assigned as "local"
����� 1 Magic variable "$"" should be assigned as "local"
����� 1 Expression form of "map"

I looked less closely at the rest, but none give me a favorable impression.

In summary, among those warnings, I see non-negative value in "Code before
warnings are enabled" only. While we're changing this, I propose removing
Subroutines::RequireFinalReturn. Implicit return values were not a material
source of PostgreSQL bugs, yet we've allowed this to litter our code:

$ find src -name '*.p[lm]'| xargs grep -n '^.return;' | wc -l
194

On 2020-04-11 06:30, Noah Misch wrote:

In summary, among those warnings, I see non-negative value in "Code before
warnings are enabled" only.

Now that you put it like this, that was also my impression when I first
introduced the level 5 warnings and then decided to stop there.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Noah Misch <noah@leadboat.com> writes:

In summary, among those warnings, I see non-negative value in "Code before
warnings are enabled" only. While we're changing this, I propose removing
Subroutines::RequireFinalReturn.

If it's possible to turn off just that warning, then +several.
It's routinely caused buildfarm failures, yet I can detect exactly
no value in it. If there were sufficient cross-procedural analysis
backing it to detect whether any caller examines the subroutine's
result value, then it'd be worth having. But there isn't, so those
extra returns are just pedantic verbosity.

regards, tom lane

On 4/11/20 12:30 AM, Noah Misch wrote:

On Thu, Apr 09, 2020 at 11:44:11AM -0400, Andrew Dunstan wrote:

     39 Always unpack @_ first

Requiring a "my @args = @_" does not improve this code:

sub CreateSolution
{
...
if ($visualStudioVersion eq '12.00')
{
return new VS2013Solution(@_);
}

     30 Code before warnings are enabled

Sounds good. We already require "use strict" before code. Requiring "use
warnings" in the exact same place does not impose much burden.

     12 Subroutine "new" called using indirect syntax

No, thanks. "new VS2013Solution(@_)" and "VS2013Solution->new(@_)" are both
fine; enforcing the latter is an ongoing waste of effort.

      9 Multiple "package" declarations

This is good advice if you're writing for CPAN, but it would make PostgreSQL
code worse by having us split affiliated code across multiple files.

      9 Expression form of "grep"

No, thanks. I'd be happier with the opposite, requiring grep(/x/, $arg)
instead of grep { /x/ } $arg. Neither is worth enforcing.

      7 Symbols are exported by default

This is good advice if you're writing for CPAN. For us, it just adds typing.

      5 Warnings disabled
      4 Magic variable "$/" should be assigned as "local"
      4 Comma used to separate statements
      2 Readline inside "for" loop
      2 Pragma "constant" used
      2 Mixed high and low-precedence booleans
      2 Don't turn off strict for large blocks of code
      1 Magic variable "@a" should be assigned as "local"
      1 Magic variable "$|" should be assigned as "local"
      1 Magic variable "$\" should be assigned as "local"
      1 Magic variable "$?" should be assigned as "local"
      1 Magic variable "$," should be assigned as "local"
      1 Magic variable "$"" should be assigned as "local"
      1 Expression form of "map"

I looked less closely at the rest, but none give me a favorable impression.

I don't have a problem with some of this. OTOH, it's nice to know what
we're ignoring and what we're not.

What I have prepared is first a patch that lowers the severity level to
3 but implements policy exceptions so that nothing is broken. Then 3
patches. One fixes the missing warnings pragma and removes shebang -w
switches, so we are quite consistent about how we do this. I gather we
are agreed about that one. The next one fixes those magic variable
error. That includes using some more idiomatic perl, and in one case
just renaming a couple of variables that are fairly opaque anyway. The
last one fixes the mixture of high and low precedence boolean operators,
the inefficient <FOO> inside a foreach loop,  and the use of commas to
separate statements, and relaxes the policy about large blocks with 'no
strict'.

Since I have written them they are attached, for posterity if nothing
else. :-)

In summary, among those warnings, I see non-negative value in "Code before
warnings are enabled" only. While we're changing this, I propose removing
Subroutines::RequireFinalReturn. Implicit return values were not a material
source of PostgreSQL bugs, yet we've allowed this to litter our code:

That doesn't mean it won't be a source of problems in future, I've
actually been bitten by this in the past.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Apr 11, 2020, at 9:13 AM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote:

Hi Andrew. I appreciate your interest and efforts here. I hope you don't mind a few questions/observations about this effort:

The
last one fixes the mixture of high and low precedence boolean operators,

I did not spot examples of this in your diffs, but I assume you mean to prohibit conditionals like:

if ($a || $b and $c || $d)

As I understand it, perl introduced low precedence operators precisely to allow this. Why disallow it?

and the use of commas to separate statements

I don't understand the prejudice against commas used this way. What is wrong with:

$i++, $j++ if defined $k;

rather than:

if (defined $k)
{
$i++;
$j++;
}

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 4/11/20 12:28 PM, Mark Dilger wrote:

On Apr 11, 2020, at 9:13 AM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote:

Hi Andrew. I appreciate your interest and efforts here. I hope you don't mind a few questions/observations about this effort:

Not at all.

The
last one fixes the mixture of high and low precedence boolean operators,

I did not spot examples of this in your diffs, but I assume you mean to prohibit conditionals like:

if ($a || $b and $c || $d)

As I understand it, perl introduced low precedence operators precisely to allow this. Why disallow it?

The docs say:

Conway advises against combining the low-precedence booleans ( |and
or not| ) with the high-precedence boolean operators ( |&& || !| )
in the same expression. Unless you fully understand the differences
between the high and low-precedence operators, it is easy to
misinterpret expressions that use both. And even if you do
understand them, it is not always clear if the author actually
intended it.

|next| |if| |not ||$foo| ||| ||$bar||;  ||#not ok|
|next| |if| |!||$foo| ||| ||$bar||;     ||#ok|
|next| |if| |!( ||$foo| ||| ||$bar| |); ||#ok|

I don't feel terribly strongly about it, but personally I just about
never use the low precendence operators, and mostly prefer to resolve
precedence issue with parentheses.

and the use of commas to separate statements

I don't understand the prejudice against commas used this way. What is wrong with:

$i++, $j++ if defined $k;

rather than:

if (defined $k)
{
$i++;
$j++;
}

I don't think the example is terribly clear. I have to look at it and
think "Does it do $i++ if $k isn't defined?"

In the cases we actually have there isn't even any shorthand advantage
like this. There are only a couple of cases.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 4/11/20 12:30 AM, Noah Misch wrote:

In summary, among those warnings, I see non-negative value in "Code before
warnings are enabled" only. While we're changing this, I propose removing
Subroutines::RequireFinalReturn. Implicit return values were not a material
source of PostgreSQL bugs, yet we've allowed this to litter our code:

That doesn't mean it won't be a source of problems in future, I've
actually been bitten by this in the past.

Yeah, as I recall, the reason for the restriction is that if you fall out
without a "return", what's returned is the side-effect value of the last
statement, which might be fairly surprising. Adding explicit "return;"
guarantees an undef result. So when this does prevent a bug it could
be a pretty hard-to-diagnose one. The problem is that it's a really
verbose/pedantic requirement for subs that no one ever examines the
result value of.

Is there a way to modify the test so that it only complains when
the final return is missing and there are other return(s) with values?
That would seem like a more narrowly tailored check.

regards, tom lane

On 4/11/20 12:48 PM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 4/11/20 12:30 AM, Noah Misch wrote:

In summary, among those warnings, I see non-negative value in "Code before
warnings are enabled" only. While we're changing this, I propose removing
Subroutines::RequireFinalReturn. Implicit return values were not a material
source of PostgreSQL bugs, yet we've allowed this to litter our code:

That doesn't mean it won't be a source of problems in future, I've
actually been bitten by this in the past.

Yeah, as I recall, the reason for the restriction is that if you fall out
without a "return", what's returned is the side-effect value of the last
statement, which might be fairly surprising. Adding explicit "return;"
guarantees an undef result. So when this does prevent a bug it could
be a pretty hard-to-diagnose one. The problem is that it's a really
verbose/pedantic requirement for subs that no one ever examines the
result value of.

Is there a way to modify the test so that it only complains when
the final return is missing and there are other return(s) with values?
That would seem like a more narrowly tailored check.

Not AFAICS:
<https://metacpan.org/pod/Perl::Critic::Policy::Subroutines::RequireFinalReturn&gt;

That would probably require writing a replacement module. Looking at the
source if this module I think it might be possible, although I don't
know much of the internals of perlcritic.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Apr 11, 2020, at 9:47 AM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote:

On 4/11/20 12:28 PM, Mark Dilger wrote:

On Apr 11, 2020, at 9:13 AM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote:

Hi Andrew. I appreciate your interest and efforts here. I hope you don't mind a few questions/observations about this effort:

Not at all.

The
last one fixes the mixture of high and low precedence boolean operators,

I did not spot examples of this in your diffs, but I assume you mean to prohibit conditionals like:

if ($a || $b and $c || $d)

As I understand it, perl introduced low precedence operators precisely to allow this. Why disallow it?

The docs say:

Conway advises against combining the low-precedence booleans ( |and
or not| ) with the high-precedence boolean operators ( |&& || !| )
in the same expression. Unless you fully understand the differences
between the high and low-precedence operators, it is easy to
misinterpret expressions that use both. And even if you do
understand them, it is not always clear if the author actually
intended it.

|next| |if| |not ||$foo| ||| ||$bar||; ||#not ok|
|next| |if| |!||$foo| ||| ||$bar||; ||#ok|
|next| |if| |!( ||$foo| ||| ||$bar| |); ||#ok|

I don't think any of those three are ok, from a code review perspective, but it's not because high and low precedence operators were intermixed.

and the use of commas to separate statements

I don't understand the prejudice against commas used this way. What is wrong with:

$i++, $j++ if defined $k;

rather than:

if (defined $k)
{
$i++;
$j++;
}

I don't think the example is terribly clear. I have to look at it and
think "Does it do $i++ if $k isn't defined?"

It works like the equivalent C-code:

if (k)
i++, j++;

which to my eyes is also fine.

I'm less concerned with which perlcritic features you enable than I am with accidentally submitting perl which looks fine to me but breaks the build. I mostly use perl from within TAP tests, which I run locally before submission to the project. Can your changes be integrated into the TAP_TESTS makefile target so that I get local errors about this stuff and can fix it before submitting a regression test to -hackers?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 4/11/20 12:48 PM, Tom Lane wrote:

Is there a way to modify the test so that it only complains when
the final return is missing and there are other return(s) with values?
That would seem like a more narrowly tailored check.

Not AFAICS:
<https://metacpan.org/pod/Perl::Critic::Policy::Subroutines::RequireFinalReturn&gt;

Yeah, the list of all policies in the parent page doesn't offer any
promising alternatives either :-(

BTW, this bit in the policy's man page seems pretty disheartening:

Be careful when fixing problems identified by this Policy; don't
blindly put a return; statement at the end of every subroutine.

since I'd venture that's *exactly* what we've done every time perlcritic
moaned about this. I wonder what else the author expected would happen.

That would probably require writing a replacement module. Looking at the
source if this module I think it might be possible, although I don't
know much of the internals of perlcritic.

I doubt we want to go maintaining our own perlcritic policies; aside from
the effort involved, it'd become that much harder for anyone to reproduce
the results.

regards, tom lane

Mark Dilger <mark.dilger@enterprisedb.com> writes:

I'm less concerned with which perlcritic features you enable than I am with accidentally submitting perl which looks fine to me but breaks the build. I mostly use perl from within TAP tests, which I run locally before submission to the project. Can your changes be integrated into the TAP_TESTS makefile target so that I get local errors about this stuff and can fix it before submitting a regression test to -hackers?

As far as that goes, I think crake is just running

src/tools/perlcheck/pgperlcritic

which you can do for yourself as long as you've got perlcritic
installed.

regards, tom lane

On Sat, Apr 11, 2020 at 11:14:52AM -0400, Tom Lane wrote:

Noah Misch <noah@leadboat.com> writes:

In summary, among those warnings, I see non-negative value in "Code before
warnings are enabled" only. While we're changing this, I propose removing
Subroutines::RequireFinalReturn.

If it's possible to turn off just that warning, then +several.

We'd not get that warning if src/tools/perlcheck/pgperlcritic stopped enabling
it by name, so it is possible to turn off by removing lines from that config.

It's routinely caused buildfarm failures, yet I can detect exactly
no value in it. If there were sufficient cross-procedural analysis
backing it to detect whether any caller examines the subroutine's
result value, then it'd be worth having. But there isn't, so those
extra returns are just pedantic verbosity.

Agreed.

On Sat, Apr 11, 2020 at 12:13:08PM -0400, Andrew Dunstan wrote:

--- a/src/tools/msvc/Project.pm
+++ b/src/tools/msvc/Project.pm
@@ -420,13 +420,10 @@ sub read_file
{
my $filename = shift;
my $F;
-	my $t = $/;
-
-	undef $/;
+	local $/ = undef;
open($F, '<', $filename) || croak "Could not open file $filename\n";
my $txt = <$F>;
close($F);
-	$/ = $t;

+1 for this and for the other three hunks like it. The resulting code is
shorter and more robust, so this is a good one-time cleanup. It's not
important to mandate this style going forward, so I wouldn't change
perlcriticrc for this one.

--- a/src/tools/version_stamp.pl
+++ b/src/tools/version_stamp.pl
@@ -1,4 +1,4 @@
-#! /usr/bin/perl -w
+#! /usr/bin/perl

#################################################################
# version_stamp.pl -- update version stamps throughout the source tree
@@ -21,6 +21,7 @@
#

use strict;
+use warnings;

This and the other "use warnings" additions look good. I'm assuming you'd
change perlcriticrc like this:

+[TestingAndDebugging::RequireUseWarnings]
+severity = 5

On Sat, Apr 11, 2020 at 11:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Noah Misch <noah@leadboat.com> writes:

In summary, among those warnings, I see non-negative value in "Code before
warnings are enabled" only. While we're changing this, I propose removing
Subroutines::RequireFinalReturn.

If it's possible to turn off just that warning, then +several.
It's routinely caused buildfarm failures, yet I can detect exactly
no value in it. If there were sufficient cross-procedural analysis
backing it to detect whether any caller examines the subroutine's
result value, then it'd be worth having. But there isn't, so those
extra returns are just pedantic verbosity.

We've actually gone out of our way to enable that particular warning.
See src/tools/perlcheck/perlcriticrc.

The idea of that warning is not entirely without merit, but in
practice it's usually pretty clear whether a function is intended to
return anything or not, and it's unlikely that someone is going to
rely on the return value when they really shouldn't be doing so. I'd
venture to suggest that the language is lax about this sort of thing
precisely because it isn't very important, and thus not worth
bothering users about.

I agree with Noah's comment about CPAN: it would be worth being more
careful about things like this if we were writing code that was likely
to be used by a wide variety of people and a lot of code over which we
have no control and which we do not get to even see. But that's not
the case here. It does not seem worth stressing the authors of TAP
tests over such things.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 4/12/20 3:22 PM, Robert Haas wrote:

On Sat, Apr 11, 2020 at 11:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Noah Misch <noah@leadboat.com> writes:

In summary, among those warnings, I see non-negative value in "Code before
warnings are enabled" only. While we're changing this, I propose removing
Subroutines::RequireFinalReturn.

If it's possible to turn off just that warning, then +several.
It's routinely caused buildfarm failures, yet I can detect exactly
no value in it. If there were sufficient cross-procedural analysis
backing it to detect whether any caller examines the subroutine's
result value, then it'd be worth having. But there isn't, so those
extra returns are just pedantic verbosity.

I agree with Noah's comment about CPAN: it would be worth being more
careful about things like this if we were writing code that was likely
to be used by a wide variety of people and a lot of code over which we
have no control and which we do not get to even see. But that's not
the case here. It does not seem worth stressing the authors of TAP
tests over such things.

FWIW, pgBackRest used Perl Critic when we were distributing Perl code
but stopped when our Perl code was only used for integration testing.
Perhaps that was the wrong call but we decided the extra time required
to run it was not worth the benefit. Most new test code is written in C
and the Perl test code is primarily in maintenance mode now.

When we did use Perl Critic we set it at level 1 (--brutal) and then
wrote an exception file for the stuff we wanted to ignore. The advantage
of this is that if new code violated a policy that did not already have
an exception we could evaluate it and either add an exception or modify
the code. In practice this was pretty rare, but we also had a short
excuse for many exceptions and a list of exceptions that should be
re-evaluated in the future.

About the time we introduced Perl Critic we were already considering the
C migration so most of the exceptions stayed.

Just in case it is useful, I have attached our old policy file with
exceptions and excuses (when we had one).

Regards,
--
-David
david@pgmasters.net

On 4/12/20 4:12 PM, David Steele wrote:

On 4/12/20 3:22 PM, Robert Haas wrote:

On Sat, Apr 11, 2020 at 11:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Noah Misch <noah@leadboat.com> writes:

In summary, among those warnings, I see non-negative value in "Code
before
warnings are enabled" only.  While we're changing this, I propose
removing
Subroutines::RequireFinalReturn.

If it's possible to turn off just that warning, then +several.
It's routinely caused buildfarm failures, yet I can detect exactly
no value in it.  If there were sufficient cross-procedural analysis
backing it to detect whether any caller examines the subroutine's
result value, then it'd be worth having.  But there isn't, so those
extra returns are just pedantic verbosity.

I agree with Noah's comment about CPAN: it would be worth being more
careful about things like this if we were writing code that was likely
to be used by a wide variety of people and a lot of code over which we
have no control and which we do not get to even see. But that's not
the case here. It does not seem worth stressing the authors of TAP
tests over such things.

FWIW, pgBackRest used Perl Critic when we were distributing Perl code
but stopped when our Perl code was only used for integration testing.
Perhaps that was the wrong call but we decided the extra time required
to run it was not worth the benefit. Most new test code is written in
C and the Perl test code is primarily in maintenance mode now.

When we did use Perl Critic we set it at level 1 (--brutal) and then
wrote an exception file for the stuff we wanted to ignore. The
advantage of this is that if new code violated a policy that did not
already have an exception we could evaluate it and either add an
exception or modify the code. In practice this was pretty rare, but we
also had a short excuse for many exceptions and a list of exceptions
that should be re-evaluated in the future.

About the time we introduced Perl Critic we were already considering
the C migration so most of the exceptions stayed.

Just in case it is useful, I have attached our old policy file with
exceptions and excuses (when we had one).

That's a pretty short list for --brutal, well done. I agree there is
value in keeping documented the policies you're not complying with.
Maybe the burden of that is too much for this use, that's up to the
project to decide.

For good or ill we now have a significant investment in perl code - I
just looked and it's 180 files with 38,135 LOC, and that's not counting
the catalog data files, so we have some interest in keeping it fairly clean.

I did something similar to what's above with the buildfarm code,
although on checking now I find it's a bit out of date for the sev 1 and
2 warnings, so I'm fixing that. Having said that, my normal target is
level 3.

The absolutely minimal things I want to do are a) fix the code that
we're agreed on fixing (use of warnings, idiomatic use of $/), and b)
fix the output format to include the name of the policy being violated.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services