Re: Inconvenience of pg_read_binary_file()

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> - Simplified the implementation (by complexifying argument handling..).
> - REVOKEd EXECUTE from the new functions.
> - Edited the signature of the two functions.

>> - pg_read_file ( filename text [, offset bigint, length bigint [, missing_ok boolean ]] ) $B"*(B text
>> + pg_read_file ( filename text [, offset bigint, length bigint ] [, missing_ok boolean ] ) $B"*(B text

I'm okay with allowing this variant of the functions.  Since there's
no implicit cast between bigint and bool, plus the fact that you
can't give just offset without length, there shouldn't be much risk
of confusion as to which variant to invoke.

I don't really like the implementation style though.  That mess of
PG_NARGS tests is illegible code already and this makes it worse.
I think it'd be way cleaner to have all the PG_GETARG calls in the
bottom SQL-callable functions (which are already one-per-signature)
and then pass them on to a common function that has an ordinary C
call signature, along the lines of

static Datum
pg_read_file_common(text *filename_t,
                    int64 seek_offset, int64 bytes_to_read,
                    bool read_to_eof, bool missing_ok)
{
    if (read_to_eof)
        bytes_to_read = -1;    // or just Assert that it's -1 ?
    else if (bytes_to_read < 0)
        ereport(...);
    ...
}

Datum
pg_read_file_off_len(PG_FUNCTION_ARGS)
{
    text       *filename_t = PG_GETARG_TEXT_PP(0);
    int64       seek_offset = PG_GETARG_INT64(1);
    int64       bytes_to_read = PG_GETARG_INT64(2);

    return pg_read_file_common(filename_t, seek_offset, bytes_to_read,
                               false, false);
}

            regards, tom lane