Config error messages

[fluca1978]

As per discussion #199, this implements a better warning message for limits that go beyond the availability of connections.

The end result is:

DEBUG configuration.c:1314 limit entry 0 with max_size = 50 exceeds remaining available connections 10. Line: pgbench pgbench 50 10  5

WARN  configuration.c:1320 max_size greater than remaining available connections at entry 1 (line 2 of file /etc/pgagroal/pgagroal_databases.conf), adjusting max_size to zero for this entry

FATAL configuration.c:1388 max_size must be greater than 0 for limit entry 1

the DEBUG and WARN are added messages that explain when the user should look at to fix the problem. Only one warning message is printed out, even if the subsequent entries are zeroed too.
This attaches also a lineno field to the limit struct to keep track of the configruation line, in order to help the user to fix the problem.

[jesperpedersen]

I think one error message is enough so bail as soon we know the configuration is invalid. I think the entry number is enough as well, so no need for the line number integration for now

[fluca1978]

I think one error message is enough so bail as soon we know the configuration is invalid. I think the entry number is enough as well, so no need for the line number integration for now

Uhm...I must confess that I do like programs that tell me what is happening and where, that's why I introduced line numbers that are less obscure that entry numbers. And that's also why I placed the filename in the error messages, so that the user is not mislead to a wrong path in the case of multiple installations.
Let me know what you think about.

Besides, the only thing that we can do here is to abort immediately the parsing of the configuration as soon as we find out there is a zeroed entry, but I don't like the idea very much. In any case, the real problem will remain hidden to the user, who will "see" only that an entry is zero when its configuration file says the opposite.

[jesperpedersen]

Yeah, I like the idea of keeping the line number but just record it in pgagroal_read_limit_configuration and then output what is wrong in pgagroal_validate_limit_configuration where logging is available.

FATAL: max_connections exceeded with LIMIT entry %d (line %d)
WARN: LIMIT entry %d (line %d) not handled
WARN: LIMIT entry %d (line %d) not handled

or something like that. We know that a 0 entry is invalid so they can be left out - e.g. just listed with the above WARN.

[fluca1978]

I've improved a little more, but now only the first zeroed entry is logged, because I don't do an extra loop to verify all the entries in the validate function.
Also it is not clear if you want to remove also the pre-fatal warning that tells the users has more limits than max_connections. So far it logs something like:

WARN  configuration.c:1356 server_max = 2 is less than required 204 max connections defined in limit file /etc/pgagroal/pgagroal_databases.conf. Adjust your configuration.
FATAL configuration.c:1383 max_size must be greater than 0 for limit entry 1
WARN  configuration.c:1384 Limit entry 1 (line 2) not handled

[jesperpedersen]

Well, the FATAL message is the important one so you could just have everything in there:

Limit entry %d exceed max_connections (%d > %d). Please, adjust your configuration in %s:%d

->

Limit entry 1 exceed max_connections (204 > 200). Please, adjust your configuration in /etc/pgagroal/pgagroal_databases.conf:2

[jesperpedersen]

Remember to remove your unrelated changes (spaces, indentation, pool.c, ...)

[fluca1978]

Well, the FATAL message is the important one so you could just have everything in there:

Sorry, but we cannot: the math about connection limits is done during the configuration parsing, i.e., at pgagroal_read_limit_configuration, but the FATAL error message is fired later, when pgagroal_validate_limit_configuration is executed and thus when all details about the exceeding limits have been already zeroed.

I see two possible solutions here:

1. config handles an error flag and an error_message, so that we can set it during parsing and log whenever we want. But this requires extra-memory, even if having an error in configuration means the application will abort and thus, when well configured, such memory will be meaningless;
2. config->max_size is made negative by the number of missing connections, therefore not zeroed, so that we can log FATAL to the user that she is missing config->max_size * -1 connections at least. Since we are going to abort the application, I think it's safe to have a negative value that will not trigger any other error anyway.

I like the (1) the most.

[jesperpedersen]

Or just have pgagroal_read_limit_configuration do pure parsing - e.g. no validation. Of course invalid parameters like XYZ should turn into its invalid representation like -1. So, I would remove https://github.com/agroal/pgagroal/blob/master/src/libpgagroal/configuration.c#L1296 and cover the case where server_max is negative in validation (the all case).

Then pgagroal_validate_limit_configuration will have access to all parsed data.

[fluca1978]

It could be simpler: if we remove the zeroing of the entries, as you suggested, the error message becomes pgagroal: LIMIT: Too many connections defined 204 (max 2) immeditaly, so there is no need to specify anything more.
)'ve also added the line numbers in validating error messages.
Could be that simple?

[jesperpedersen]

Yeah, it could.

I think it is a good idea to add the file name (and line number) to each of the WARNs / FATALs so (%s:%d) instead of (line %d).

Let me know if you want a detailed review but it is mostly white-space / indentation / ... related. E.g. remove all changes that isn't necessary for this patch.

Thanks, Luca !

[fluca1978]

It should be fine now with spaces and file/line number in limit warning messages.
I've also added a couple of curlies so that

if (!file)
   return 1;

looks like other ifs

if (!file)
{
   return 1;
}

then I realized that all other _read_ functions don't have curlies in such test. Is this correct?

[jesperpedersen]

Both are ok for single statement... But latter reads better I think

[jesperpedersen]

You need to file an enhancement issue with a description of what this pull request is trying to solve, and then use that issue number in the first line of the commit message

[fluca1978]

Issue #205 refers to discussion #199 and this pull request.
I hope spaces are fine now.

[jesperpedersen]

Looks good, but you need to rebase against master to get the latest changes in, and ideally squash your commits.

(Note, you have some merge branches in your development history)

[fluca1978]

I've rebased against master, but I'm not able to squash since I cannot merge, you need to merge and squash in the case it is fine this PR.

[jesperpedersen]

Looks like you merged master into your branch instead of rebasing on it, so I took your changes and created a patch manually.

I have

git remote add upstream https://github.com/agroal/pgagroal.git

Then I do

git fetch upstream # Get latest from repo
git rebase -i upstream/master # Rebase current branch upon master
# Do development
git commit -a -m "MyMessage"
# If I'm happy with that and it isn't the first commit then
git rebase -i HEAD~2 # Squash commit(s) into one
# Finally
git push -f origin mybranch # Force push to origin

This makes it easier for both reviewing the patch, and using git format-patch - depending on what the project in question need.

Thanks for your contribution !