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
This should also be covered in the regression tests.
I'm wondering if psql's parse_identifier() could be exported and reused here
rather than creating yet another version.
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)
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?
+ /*
+ * Release allocated sources for filter
+ */
+ void
+ filter_free_sources(FilterStateData *fstate)
I'm assuming "ressources" not "sources"?
+ /*
+ * 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)
+ static const char *
+ filter_object_type_name(FilterObjectType fot)
No description.
/*
* Helper routine to reduce duplicated code
*/
void
log_unsupported_filter_object_type(FilterStateData *fstate,
const char *appname,
FilterObjectType fot)
Need more helpful comment.
