Re: proposal: possibility to read dumped table's name from file

Hi


út 18. 10. 2022 v 11:33 odesílatel Julien Rouhaud <rjuju123@gmail.com>
napsal:

> On Thu, Oct 13, 2022 at 11:46:34AM -0700, Andres Freund wrote:
> > Hi,
> >
> > On 2022-10-07 07:26:08 +0200, Pavel Stehule wrote:
> > > I am sending version with handy written parser and meson support
> >
> > Given this is a new approach it seems inaccurate to have the CF entry
> marked
> > ready-for-committer. I've updated it to needs-review.
>
> I just had a quick look at the rest of the patch.
>
> For the parser, it seems that filter_get_pattern is reimplementing an
> identifier parsing function but isn't entirely correct.  It can correctly
> parse
> quoted non-qualified identifiers and non-quoted qualified identifiers, but
> not
> quoted and qualified ones.  For instance:
>
> $ echo 'include table nsp.tbl' | pg_dump --filter - >/dev/null
> $echo $?
> 0
>
> $ echo 'include table "TBL"' | pg_dump --filter - >/dev/null
> $echo $?
> 0
>
> $ echo 'include table "NSP"."TBL"' | pg_dump --filter - >/dev/null
> pg_dump: error: invalid format of filter on line 1: unexpected extra data
> after pattern
>

fixed


>
> This should also be covered in the regression tests.
>

done


>
> I'm wondering if psql's parse_identifier() could be exported and reused
> here
> rather than creating yet another version.
>

I looked there, and I don't think this parser is usable for this purpose.
It is  very sensitive on white spaces, and doesn't support multi-lines. It
is designed for support readline tab complete, it is designed for
simplicity not for correctness.


>
> Nitpicking: the comments needs some improvements:
>
> + /*
> +  * Simple routines - just don't repeat same code
> +  *
> +  * Returns true, when filter's file is opened
> +  */
> + bool
> + filter_init(FilterStateData *fstate, const char *filename)
>

done


>
> also, is there any reason why this function doesn't call exit_nicely in
> case of
> error rather than letting each caller do it without any other cleanup?
>

It is commented few lines up

/*
 * Following routines are called from pg_dump, pg_dumpall and pg_restore.
 * Unfortunatelly, implementation of exit_nicely in pg_dump and pg_restore
 * is different from implementation of this rutine in pg_dumpall. So instead
 * direct calling exit_nicely we have to return some error flag (in this
 * case NULL), and exit_nicelly will be executed from caller's routine.
 */


>
> + /*
> +  * Release allocated sources for filter
> +  */
> + void
> + filter_free_sources(FilterStateData *fstate)
>
> I'm assuming "ressources" not "sources"?
>

changed


>
> + /*
> +  * log_format_error - Emit error message
> +  *
> +  * This is mostly a convenience routine to avoid duplicating file
> closing code
> +  * in multiple callsites.
> +  */
> + void
> + log_invalid_filter_format(FilterStateData *fstate, char *message)
>
> mismatch between comment and function name (same for filter_read_item)
>

fixes


>
> + static const char *
> + filter_object_type_name(FilterObjectType fot)
>
> No description.
>
>
fixed


> /*
>  * Helper routine to reduce duplicated code
>  */
> void
> log_unsupported_filter_object_type(FilterStateData *fstate,
>
> const char *appname,
>
> FilterObjectType fot)
>
> Need more helpful comment.
>

fixed

Thank you for comments

attached updated patch

Regards

Pavel