agroal / pgagroal

High-performance connection pool for PostgreSQL
https://agroal.github.io/pgagroal/
BSD 3-Clause "New" or "Revised" License
685 stars 61 forks source link

Redo CLI conf get|set|ls #485

Open jesperpedersen opened 2 weeks ago

fluca1978 commented 2 weeks ago

Testing head f34e4f93d8e0e93ccf2f403ed486eb088fb8cd79:

% pgagroal-cli conf get max_connections
pgagroal-cli: Unknown subcommand 'get' for command 'conf'

and most notably:

% pgagroal-cli conf reload             
-> WARN  management.c:1162 pgagroal-cli: read_byte: (nil) 3 Invalid argument
pgagroal-cli: No connection to pgagroal on /tmp

and for the last command conf reload the pgagroal logs show:

-> DEBUG main.c:1444 Management 12: {
  "Header": {
    "ClientVersion": "2.0.0",
    "Command": 12,
    "Compression": 0,
    "Encryption": 0,
    "Output": 0,
    "Timestamp": "20241113104305"
  },
  "Request": {}
}
-> DEBUG main.c:1776 pgagroal: Management reload
pgagroal: main: Unknown key <ev_backend> with value <io_uring> in section [pgagroal] (line 80 of file </etc/pgagroal/pgagroal.conf>)
-> DEBUG configuration.c:2016 Reload: Success
-> DEBUG main.c:2569 Socket: 7
-> DEBUG main.c:2569 Socket: 8
-> DEBUG main.c:2571 Unix Domain Socket: 5
-> DEBUG main.c:2574 Metrics: 11
-> DEBUG main.c:2574 Metrics: 12
-> DEBUG main.c:2578 Remote management: 13
-> DEBUG main.c:2578 Remote management: 14
fluca1978 commented 2 weeks ago

With regard to the conf reload, I found that here https://github.com/agroal/pgagroal/blob/master/src/main.c#L2586 there is an explicit exit(0) (and similarly exit(1) on the error path), that is also not consistent with the function defintion that is supposed to return a bool.

fluca1978 commented 2 weeks ago

@jesperpedersen PTAL commit ea07bd4 that implements conf get to see if I get it right.

Please note that there is some sort of user after free bug somewhere, but I've not digged it a lot. It could be that destroying the JSON object after writing the response suffice. Take a look at the following two executions: the first is ok, the secondis in error but reports the same configuration value for the requested key.

% pgagroal-cli conf get log_level 
Header: 
  ClientVersion: 2.0.0
  Command: 2
  Compression: none
  Encryption: none
  Output: text
  Timestamp: 20241113123146
Outcome: 
  Status: true
  Time: 00:00:00
Request: 
  ConfigurationKey: log_level
Response: 
  ServerVersion: 2.0.0
  log_level: debug

% pgagroal-cli conf get log_     
Header: 
  ClientVersion: 2.0.0
  Command: 2
  Compression: none
  Encryption: none
  Output: text
  Timestamp: 20241113123152
Outcome: 
  Error: 1
  Status: false
Request: 
  ConfigurationKey: log_
Response: 
  ServerVersion: 2.0.0
  log_: debug
jesperpedersen commented 2 weeks ago

@fluca1978 All the CONF_XYZ work will be ported from https://github.com/pgmoneta/pgmoneta/pull/409 once done.

fluca1978 commented 2 weeks ago

@fluca1978 All the CONF_XYZ work will be ported from pgmoneta/pgmoneta#409 once done.

Ok, so this is a "phantom" issue. Hope at least my initial work can be useful.

jesperpedersen commented 2 weeks ago

Yeah, so it doesn't fly out of my head and we have a feature regression