Re: Inconvenience of pg_read_binary_file()

Thanks for taking a look!

At Thu, 28 Jul 2022 16:22:17 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> 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 ]] ) → text
> >> + pg_read_file ( filename text [, offset bigint, length bigint ] [, missing_ok boolean ] ) → 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.

Grad to hear that.

> I don't really like the implementation style though.  That mess of
> PG_NARGS tests is illegible code already and this makes it worse.

Ah..., I have to admit that I faintly felt that feeling while on it...

> 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 ?

I prefer assertion since that parameter cannot be passed by users.

>     else if (bytes_to_read < 0)
>         ereport(...);
>     ...
> }

This function cannot return NULL directly. Without the ability to
return NULL, it is pointless for the function to return Datum.  In the
attached the function returns text*.

> 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);

As the result this function need to return NULL or TEXT_P according to
the returned value from pg_read_file_common.

+    if (!ret)
+        PG_RETURN_NULL();
+
+    PG_RETURN_TEXT_P(ret);
> }

# I'm tempted to call read_text_file() directly from each SQL functions..

Please find the attached.  I added some regression tests for both
pg_read_file() and pg_read_binary_file().

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center