Date: Mon, 13 Sep 2021 08:11:52 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Pavel Stehule <pavel.stehule@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Alvaro Herrera <alvherre@2ndquadrant.com>,
	Tomas Vondra <tomas.vondra@enterprisedb.com>,
	Daniel Gustafsson <daniel@yesql.se>,
	Dean Rasheed <dean.a.rasheed@gmail.com>,
	Stephen Frost <sfrost@snowman.net>,
	Surafel Temesgen <surafel3000@gmail.com>,
	vignesh C <vignesh21@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: proposal: possibility to read dumped table's name from file
Message-ID: <20210913131152.GO26465@telsasoft.com>
References: <202107122259.n6o5uwb5erza@alvherre.pgsql>
 <3183720.1626131795@sss.pgh.pa.us>
 <CAFj8pRBq9X=oSNbo+v93EwznkQ3JczEW6UsfQK37vx1ktW-2ZA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAFj8pRBq9X=oSNbo+v93EwznkQ3JczEW6UsfQK37vx1ktW-2ZA@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20210913131152.GO26465%40telsasoft.com>
Precedence: bulk

On Wed, Jul 28, 2021 at 09:28:17AM +0200, Pavel Stehule wrote:
> Here is an updated implementation of filter's file, that implements syntax
> proposed by you.

Thanks.

If there's any traction for this approach.  I have some comments for the next
revision,

> +++ b/doc/src/sgml/ref/pg_dump.sgml
> @@ -789,6 +789,56 @@ PostgreSQL documentation
>        </listitem>
>       </varlistentry>
>  
> +     <varlistentry>
> +      <term><option>--filter=<replaceable class="parameter">filename</replaceable></option></term>
> +      <listitem>
> +       <para>
> +        Read objects filters from the specified file.
> +        If you use "-" as a filename, the filters are read from stdin.

Say 'Specify "-" to read from stdin'

> +        The lines starting with symbol <literal>#</literal> are ignored.

Remove "The" and "symbol"

> +        Previous white chars (spaces, tabs) are not allowed. These

Preceding whitespace characters...

But actually, they are allowed?  But if it needs to be explained, maybe they
shouldn't be - I don't see the utility of it.

> +static bool
> +isblank_line(const char *line)
> +{
> +	while (*line)
> +	{
> +		if (!isblank(*line++))
> +			return false;
> +	}
> +
> +	return true;
> +}

I don't think this requires nor justifies having a separate function.
Either don't support blank lines, or use get_keyword() with size==0 for that ?

> +		/* Now we expect sequence of two keywords */
> +		if (keyword && is_keyword(keyword, size, "include"))
> +			is_include = true;
> +		else if (keyword && is_keyword(keyword, size, "exclude"))
> +			is_include = false;
> +		else

I think this should first check "if keyword == NULL".
That could give a more specific error message like "no keyword found",

> +			exit_invalid_filter_format(fp,
> +									   filename,
> +									   "expected keyword \"include\" or \"exclude\"",
> +									   line.data,
> +									   lineno);

..and then this one can say "invalid keyword".

-- 
Justin