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

Hi

út 18. 10. 2022 v 11:33 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)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