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
signature.asc
Description: PGP signature