[FEATURE REQUEST] Updates to pgjsonb returner

[PaulChristophel]

I was uncertain whether to mark this as a feature request or as a bug, but I noticed a few issues with the pgjsonb returner. Forgive me if this request is way off base.

Issues with job handling

The table itself is fine, but jobs are (debatably) improperly handled.

If the server supports it, the code is set to update on conflicts

        PG_SAVE_LOAD_SQL = """INSERT INTO jids
                              (jid, load)
                              VALUES (%(jid)s, %(load)s)
                              ON CONFLICT (jid) DO UPDATE
                              SET load=%(load)s"""

Because of the way salt handles jobs, this is actually bad. I noticed that if I run a job like test.ping, the job will be put into the load column like

arg: []
cmd: publish
jid: '20240801142738891639'
fun: test.ping
ret: ''
tgt: '*'
tgt_type: glob
user: foo

which is right, but as minions start responding to the ping, this row is replaced. This wouldn't be a problem if the return were aggregated somehow, but they are not. The job data is replaced by the last minion that responds. This means the publish data is being lost along with all the other minions that respond to the job, save the last. I'm not sure how critical this is, but it seems like a bug to me. My thought would be to handle it like the postgres.py module does now and just discard subsequent updates to the row.

Issues with salt_returns handling

The salt_returns table suggested by the documentation is:

    CREATE TABLE salt_returns (
      fun varchar(50) NOT NULL,
      jid varchar(255) NOT NULL,
      return jsonb NOT NULL,
      id varchar(255) NOT NULL,
      success varchar(10) NOT NULL,
      full_ret jsonb NOT NULL,
      alter_time TIMESTAMP WITH TIME ZONE DEFAULT NOW());

1. Given that jobs are unique per minion, it would make sense to add that as a constraint to the table.
2. Per the jids table, the jid column should never exceed 20 characters.
3. Since it is already marked as NOT NULL the success column should be a boolean.

    CREATE TABLE salt_returns (
      fun varchar(50) NOT NULL,
      jid varchar(20) NOT NULL,
      return jsonb NOT NULL,
      id varchar(255) NOT NULL,
      success boolean NOT NULL,
      full_ret jsonb NOT NULL,
      alter_time TIMESTAMP WITH TIME ZONE DEFAULT NOW())
      CONSTRAINT salt_returns_pkey PRIMARY KEY (jid, id);

This change prevents lots of duplicated returns in the salt_returns table. It will require updates to the returner function though:

This:

def returner(ret):
    """
    Return data to a Pg server
    """
    try:
        with _get_serv(ret, commit=True) as cur:
            sql = """INSERT INTO salt_returns
                    (fun, jid, return, id, success, full_ret, alter_time)
                    VALUES (%s, %s, %s, %s, %s, %s, to_timestamp(%s))"""

would need to become something like:

def returner(ret):
    """
    Return data to a Pg server
    """
    try:
        with _get_serv(ret, commit=True) as cur:
            sql = """INSERT INTO salt_returns
                    (fun, jid, return, id, success, full_ret, alter_time)
                    VALUES (%s, %s, %s, %s, %s, %s, to_timestamp(%s))
                    ON CONFLICT (jid, id) DO UPDATE SET
                        fun = EXCLUDED.fun,
                        return = EXCLUDED.return,
                        success = EXCLUDED.success,
                        full_ret = EXCLUDED.full_ret,
                        alter_time = EXCLUDED.alter_time"""

or more likely, something similar to the way conflicts are handled now for the jids table.

I also noticed a lot of discrepancies between what salt considers a "success" and what actually gets marked as a success in the row. (e.g., A job will have a retcode of 0 for a highstate with no failures and still be marked with a success of false.)

I am happy to implement any/all these changes myself, but I wanted to mark this as an issue first to see if you're amenable to them.