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

čt 3. 11. 2022 v 5:09 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:

> Hi,
>
> On Wed, Oct 26, 2022 at 06:26:26AM +0200, Pavel Stehule wrote:
> >
> > út 18. 10. 2022 v 11:33 odesílatel Julien Rouhaud <rjuju123@gmail.com>
> > napsal:
> >
> > >
> > > 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.
>
> Ah, sorry I should have checked more thoroughly.  I guess it's ok to have
> another identifier parser for the include file then, as this new one
> wouldn't
> really fit the tab-completion use case.
>
> > > 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.
> >  */
>
> Oh right, I totally missed it sorry about that!
>
> About the new version, I didn't find any problem with the feature itself so
> it's a good thing!
>
> I still have a few comments about the patch.  First, about the behavior:
>
> - is that ok to have just "data" pattern instead of "table_data" or
> something
>   like that, since it's supposed to match --exclude-table-data option?
>

done


>
> - the error message are sometimes not super helpful.  For instance:
>
> $ echo "include data t1" | pg_dump --filter -
> pg_dump: error: invalid format of filter on line 1: include filter is not
> allowed for this type of object
>
> It would be nice if the error message mentioned "data" rather than a
> generic
> "this type of object".  Also, maybe we should quote "include" to outline
> that
> we found this keyword?
>
>
done



> About the patch itself:
> filter.c:
>
> +#include "postgres_fe.h"
> +
> +#include "filter.h"
> +
> +#include "common/logging.h"
>
> the filter.h inclusion should be done with the rest of the includes, in
> alphabetical order.
>
>
done




> +#define                is_keyword_str(cstr, str, bytes) \
> +       ((strlen(cstr) == bytes) && (pg_strncasecmp(cstr, str, bytes) ==
> 0))
>
> nit: our guidline is to protect macro arguments with parenthesis.  Some
> arguments can be evaluated multiple times but I don't think it's worth
> adding a
> comment for that.
>
>
done


> + * 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
>
> typos: s/Unfortunatelly/Unfortunately/ and s/rutine/routine/
> Also, it would probably be better to say "instead of directly calling..."
>
>
done


> +static const char *
> +filter_object_type_name(FilterObjectType fot)
> +{
> +       switch (fot)
> +       {
> +               case FILTER_OBJECT_TYPE_NONE:
> +                       return "comment or empty line";
> +[...]
> +       }
> +
> +       return "unknown object type";
> +}
>
> I'm wondering if we should add a pg_unreachable() there, some compilers
> might
> complain otherwise.  See CreateDestReceiver() for instance for similar
> pattern.
>

done


>
> + * Emit error message "invalid format of filter file ..."
> + *
> + * This is mostly a convenience routine to avoid duplicating file closing
> code
> + * in multiple callsites.
> + */
> +void
> +log_invalid_filter_format(FilterStateData *fstate, char *message)
>
> nit: invalid format *in* filter file...?
>

changed


>
> +void
> +log_unsupported_filter_object_type(FilterStateData *fstate,
> +
>  const char *appname,
> +
>  FilterObjectType fot)
> +{
> +       PQExpBuffer str = createPQExpBuffer();
> +
> +       printfPQExpBuffer(str,
> +                                         "The application \"%s\" doesn't
> support filter for object type \"%s\".",
>
> nit: there shouldn't be uppercase in error messages, especially since this
> will
> be appended to another message by log_invalid_filter_format.  I would just
> just
> drop "The application" entirely for brevity.
>

changed


>
> +/*
> + * Release allocated resources for filter
> + */
> +void
> +filter_free(FilterStateData *fstate)
>
> nit: Release allocated resources for *the given* filter?
>

changed

>
> + * Search for keywords (limited to ascii alphabetic characters) in
> + * the passed in line buffer. Returns NULL, when the buffer is empty or
> first
> + * char is not alpha. The length of the found keyword is returned in the
> size
> + * parameter.
> + */
> +static const char *
> +filter_get_keyword(const char **line, int *size)
> +{
> +   [...]
> +       if (isascii(*ptr) && isalpha(*ptr))
> +       {
> +               result = ptr++;
> +
> +               while (isascii(*ptr) && (isalpha(*ptr) || *ptr == '_'))
> +                       ptr++;
>
> Is there any reason to test isascii()?  isalpha() should already cover
> that and
> should be cheaper to test anyway.
>

changed. I wanted to limit keyword's char just for basic ascii alphabets,
but the benefit probably is not too strong, and the real effect can be
messy, so I removed isascii test


>
> Also nit: "Returns NULL when the buffer..." (unnecessary comma), and the
> '_'
> char is also allowed.
>

done


>
> +filter_read_item(FilterStateData *fstate,
> +                                bool *is_include,
> +                                char **objname,
> +                                FilterObjectType *objtype)
> +{
> +       Assert(!fstate->is_error);
> +
> +       if (pg_get_line_buf(fstate->fp, &fstate->linebuff))
> +       {
> +               char       *str = fstate->linebuff.data;
> +               const char *keyword;
> +               int                     size;
> +
> +               fstate->lineno++;
> +
> +               (void) pg_strip_crlf(str);
> +
> +               /* Skip initial white spaces */
> +               while (isspace(*str))
> +                       str++;
> +[...]
> +                       keyword = filter_get_keyword((const char **) &str,
> &size);
>
> Is there any interest with the initial pg_strip_crlf?  AFAICT all the rest
> of
> the code will ignore such caracters using isspace() so it wouldn't change
> anything.
>

I think reading multiline identifiers is a little bit easier, because I
don't need to check the ending \n and \r
When I read multiline identifiers, I cannot ignore white spaces.




> Dropping both pg_strip_crlf() would allow you to declare str as const
> rather
> than doing it in function calls.  It would require to add const qualifiers
> in a
> few other places, but it seems like an improvement, as for instance right
> now
> filter_get_pattern is free to rewrite the str (because it's also calling
> pg_strip_crlf, but there's no guarantee that it doesn't do anything else).
>
> +/*
> + * filter_get_pattern - Read an object identifier pattern from the buffer
> + *
> + * Parses an object identifier pattern from the passed in buffer and sets
> + * objname to a string with object identifier pattern. Returns pointer to
> the
> + * first character after the pattern. Returns NULL on error.
> + */
> +static char *
> +filter_get_pattern(FilterStateData *fstate,
>
> nit: suggestion to reword the comment, maybe something like
>
> /*
>  * filter_get_pattern - Identify an object identifier pattern
>  *
>  * Try to parse an object identifier pattern from the passed buffer.  If
> one is
>  * found, it sets objname to a string with the object identifier pattern
> and
>  * returns a pointer to the first byte after the found pattern. Otherwise
> NULL
>  * is returned.
>  */
>
>
replaced


> +bool
> +filter_read_item(FilterStateData *fstate,
>
> Another suggestion for comment rewrite:
>
> /*-------------------
>  * filter_read_item - Read command/type/pattern triplet from a filter file
>  *
>  * This will parse one filter item from the filter file, and while it is a
>  * row based format a pattern may span more than one line due to how object
>  * names can be constructed.  The expected format of the filter file is:
>  *
>  * <command> <object_type> <pattern>
>  *
>  * command can be "include" or "exclude"
>  * object_type can one of: "table", "schema", "foreign_data", "data",
>  * "database", "function", "trigger" or "index"
>  * pattern can be any possibly-quoted and possibly-qualified identifier.
> It
>  * follows the same rules as other object include and exclude functions so
> it
>  * can also use wildcards.
>  *
>  * Returns true when one filter item was successfully read and parsed.
> When
>  * object name contains \n chars, then more than one line from input file
> can
>  * be processed.  Returns false when the filter file reaches EOF. In case
> of
>  * error, the function will emit an appropriate error message before
> returning
>  * false.
>  */
>
>
replaced, thank you for the text


> Note also that your original comment said:
> + * In case of
> + * errors, the function wont return but will exit with an appropriate
> error
> + * message.
>
> But AFAICS that's not the case: it will indeed log an appropriate error
> message
> but will return false.  I'm assuming that the comment was outdated as the
> calling code handles it just fine, so I just modified the comment.
>

yes


>
> filter.h:
>
> +#ifndef FILTER_H
> +#define FILTER_H
> +#include "c.h"
>
> It's definitely not ok to include .ch in frontend code.  But AFAICS just
> removing it doesn't cause any problem.  Note also that there should be an
> empty
> line after the #define FILTER_H per usual coding style.
>

fixed - it looks so it was some garbage

updated patch attached

big thanks for these comments and tips

Regards

Pavel