Re: Allow file inclusion in pg_hba and pg_ident files

On Sat, Nov 12, 2022 at 04:13:53PM +0800, Julien Rouhaud wrote:
> It's looks good to me. I agree that file name and line number should be enough
> to diagnose any unexpected error.

Thanks for checking. I have looked at 0001 and 0002 again with a
fresh mind, and applied both of them this morning.

This makes the remaining bits of the patch much easier to follow in
hba.c. Here are more comments after a closer review of the whole for
the C logic.

-MemoryContext
-tokenize_auth_file(const char *filename, FILE *file, List **tok_lines,
- int elevel, int depth)
+static void
+tokenize_file_with_context(MemoryContext linecxt, const char *filename,

I really tend to prefer having one routine rather than two for the
tokenization entry point. Switching to the line context after setting
up the callback is better, and tokenize_file_with_context() does so.
Anyway, what do you think about having one API that gains a
"MemoryContext *" argument, as of the following:
void tokenize_auth_file(const char *filename, FILE *file,
List **tok_lines,
int depth, int elevel, MemoryContext *linectx)

If the caller passes NULL for *linectx as the initial line context,
just create it as we do now. If *linectx is not NULL, just reuse it.
That may be cleaner than returning the created MemoryContext as
returned result from tokenize_auth_file().

+ /* Cumulate errors if any. */
+ if (err_msg)
+ {
+ if (err_buf.len > 0)
+ appendStringInfoChar(&err_buf, '\n');
+ appendStringInfoString(&err_buf, err_msg);
+ }

This aggregates all the error messages for all the files included in a
given repository. As the patch stands, it seems to me that we would
get errors related to an include_dir clause for two cases:
- The specified path does not exist, in which case we have only one
err_msg to consume and report back.
- Multiple failures in opening and/or reading included files.
In the second case, aggregating the reports would provide a full set
of information, but that's not something a user would be able to act
on directly as this is system-related. Or there is a case to know a
full list of files in the case of multiple files that cannot be read
because of permission issues? We may be fine with just the first
system error here. Note that in the case where files can be read and
opened, these would have their own TokenizedAuthLines for each line
parsed, meaning one line in the SQL views once translated to an
HbaLine or an IdentLine.

This line of thoughts brings an interesting point, actually: there is
an inconsistency between "include_if_exists" and "include" compared to
the GUC processing. As of the patch, if we use "include" on a file
that does not exist, the tokenization logic would jump over it and
continue processing the follow-up entries anyway. This is a different
policy than the GUCs, where we would immediately stop looking at
parameters after an "include" if it fails because its file does not
exist, working as a immediate stop in the processing. The difference
that the patch brings between "include_if_exists" and "include" is
that we report an error in one case but not the other, still skip the
files in both cases and move on with the rest. Hence my question,
shouldn't we do like the GUC processing for the hba and ident files,
aka stop immediately when we fail to find a file on an "include"
clause? This would be equivalent to doing a "break" in
tokenize_file_with_context() after failing an include file.
--
Michael